redis-proxy: raise ElasticKV connection budget#1154
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRedis proxyの容量、二次処理の並列度と期限、ZREM replay、Lua cleanup、HLC lease更新、起動ゲートの時刻機能を変更し、関連するテスト、設定例、監視、運用ドキュメントを更新した。 ChangesRedis proxyと二次処理
Lua commit cleanup
HLC lease更新
起動ゲートの時刻機能
テスト安定化
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
@coderabbitai review |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/redis-proxy/main.go`:
- Line 58: Update the help text for the secondary-script-concurrency flag in
main so its 0-value behavior explicitly states the derived concurrency has a
minimum of 1, matching the documentation in redis-proxy-deployment.md.
🪄 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: 2c932864-260d-4d88-a336-d76f208c8dbd
📒 Files selected for processing (14)
README.mdadapter/redis.goadapter/redis_lua.goadapter/redis_lua_context.goadapter/redis_peer_limiter.goadapter/redis_peer_limiter_test.gocmd/redis-proxy/main.gocmd/redis-proxy/main_test.godeploy/redis-proxy/docker-compose.ha.ymldocs/redis-proxy-deployment.mdproxy/backend.goproxy/config.goproxy/dualwrite.goproxy/proxy_test.go
![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: b4a9a63f25
ℹ️ 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".
|
@codex review |
|
@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 (a802b7d):
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: a802b7dbab
ℹ️ 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".
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 (9dc4adf):
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 |
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 (486037c):
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
adapter/redis_lua.go (1)
836-836: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLua 互換実行経路にも Lua 専用 timeout を使ってください。
Line 836 の
execLuaCompatは Lua context と commit を実行するにもかかわらず、通常のredisDispatchTimeoutを使用しています。runLuaScriptとcommitはredisLuaDispatchTimeoutへ移行済みのため、この経路だけ短い通常期限で失敗します。修正案
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(context.Background(), redisLuaDispatchTimeout)🤖 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_lua.go` at line 836, execLuaCompat の context 作成で、通常の redisDispatchTimeout ではなく Lua 専用の redisLuaDispatchTimeout を使用してください。Lua context と commit の実行経路が runLuaScript と同じ Lua 用タイムアウトになるよう、該当する WithTimeout 呼び出しだけを更新してください。
🤖 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 `@cmd/redis-proxy/main.go`:
- Line 65: secondary-script-timeout の負値を拒否し、ヘルプの「0
のみがフォールバック」という仕様と、SecondaryScriptTimeout <= 0
をフォールバック扱いするヘルパーの挙動を一致させてください。cmd/redis-proxy/main.go の secondary-script-timeout
CLI 入力または ProxyConfig 構成時に負値を検証してエラーにし、proxy/dualwrite.go の該当ヘルパーでは 0 のみを
SecondaryTimeout へフォールバックさせてください。
In `@monitoring/hlc.go`:
- Line 71: Update the metric Help text near the wallSkewSeconds definition to
document the pre-bootstrap condition consistently with the explanation around
the physical-window metric: clarify that wallSkewSeconds > -5 is meaningful for
alerting only when the physical window metric is positive, so ceilingMs == 0 at
startup does not trigger a false alert.
In `@proxy/proxy_test.go`:
- Around line 1170-1191: Replace the fixed time.Sleep in this test with
deterministic synchronization that waits for the script to be queued, using an
appropriate channel or metric signal before releasing blocker. Ensure cleanup is
registered to always unblock the blocked d.goWrite goroutine and call d.Close,
including when the test fails, using sync.Once or equivalent.
---
Outside diff comments:
In `@adapter/redis_lua.go`:
- Line 836: execLuaCompat の context 作成で、通常の redisDispatchTimeout ではなく Lua 専用の
redisLuaDispatchTimeout を使用してください。Lua context と commit の実行経路が runLuaScript と同じ
Lua 用タイムアウトになるよう、該当する WithTimeout 呼び出しだけを更新してください。
🪄 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: 2c1c92b6-6eaa-483e-8fbf-4deda4f72d07
📒 Files selected for processing (20)
README.mdadapter/redis_compat_helpers.goadapter/redis_lua.goadapter/redis_lua_context.goadapter/redis_txn_test.gocmd/redis-proxy/main.gocmd/redis-proxy/main_test.godocs/architecture_overview.mddocs/redis-proxy-deployment.mdkv/coordinator.gokv/sharded_coordinator.gomain.gomain_encryption_rotate_on_startup_test.gomonitoring/hlc.gomonitoring/hlc_test.goproxy/async_queue.goproxy/backend.goproxy/config.goproxy/dualwrite.goproxy/proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- proxy/backend.go
- docs/redis-proxy-deployment.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 486037c994
ℹ️ 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".
| defaultDialTimeout = 5 * time.Second | ||
| defaultReadTimeout = 3 * time.Second | ||
| blockingReadGrace = 10 * time.Second | ||
| defaultElasticKVReadTimeout = defaultSecondaryScriptTimeout + blockingReadGrace |
There was a problem hiding this comment.
When an operator sets -secondary-script-timeout above the 5-minute default, this fixed default still leaves the ElasticKV go-redis client's ReadTimeout at 5m10s because cmd/redis-proxy builds secondaryOpts from DefaultElasticKVBackendOptions() and never adjusts it from the parsed config. go-redis applies that read deadline to Do calls, so async EVAL/EVALSHA replays can fail with a socket timeout before the advertised script deadline; derive the read timeout from the resolved script timeout plus grace, or reject larger values.
Useful? React with 👍 / 👎.
| func DefaultElasticKVBackendOptions() BackendOptions { | ||
| opts := DefaultBackendOptions() | ||
| opts.PoolSize = defaultElasticKVPoolSize | ||
| opts.ReadTimeout = defaultElasticKVReadTimeout |
There was a problem hiding this comment.
In elastickv-primary and elastickv-only modes the ElasticKV backend is the synchronous primary, but this default is also applied there, so ordinary client reads/writes now inherit the 5m10s script socket deadline. The proxy's foreground handlers call the backend with context.Background(), so a stalled ElasticKV GET/SET can block the client for minutes instead of the previous 3s; use the long timeout only for secondary script replays, or add a separate per-command timeout for foreground traffic.
Useful? React with 👍 / 👎.
| flag.IntVar(&secondaryBlockingReplayQueueSize, "secondary-blocking-replay-queue-size", secondaryBlockingReplayQueueSize, "Maximum queued asynchronous secondary mutating blocking-command replays (0 = derived from blocking replay concurrency)") | ||
| flag.StringVar(&modeStr, "mode", "dual-write", "Proxy mode: redis-only, dual-write, dual-write-shadow, elastickv-primary, elastickv-only") | ||
| flag.DurationVar(&cfg.SecondaryTimeout, "secondary-timeout", cfg.SecondaryTimeout, "Secondary write timeout") | ||
| flag.DurationVar(&cfg.SecondaryScriptTimeout, "secondary-script-timeout", cfg.SecondaryScriptTimeout, "Secondary Lua-script write timeout (0 = secondary-timeout)") |
There was a problem hiding this comment.
In elastickv-primary mode the async secondary for script replays is Redis (primaryOpts), whose backend options still keep the 3s ReadTimeout; this new -secondary-script-timeout only changes the task context. A Lua script that succeeds on ElasticKV but takes longer than 3s to replay to Redis will still hit go-redis' socket deadline and be recorded as a secondary write failure despite the advertised 5m default, so the Redis backend needs a secondary-specific read timeout when it is used for async script replay.
Useful? React with 👍 / 👎.
|
@codex review Latest-head follow-up:
Validation:
Latest pushed head: |
|
Codex Review: Didn't find any major issues. Swish! 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". |
…onnection-budget # Conflicts: # docs/redis-proxy-deployment.md # proxy/backend.go # proxy/dualwrite.go # proxy/leader_aware_backend.go
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 (9a94c9b):
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. |
|
Evidence for current HEAD 9a94c9b:
Validation:
Current-head CI is running after the push. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a94c9b522
ℹ️ 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 (b *LeaderAwareRedisBackend) doWithReadTimeoutOnce(ctx context.Context, timeout time.Duration, args ...any) *redis.Cmd { | ||
| cli := b.currentClient() |
There was a problem hiding this comment.
When an async single-command replay reaches a newly constructed leader-aware ElasticKV backend before the initial INFO discovery completes, secondaryDo now calls DoWithReadTimeout, but this path only reads currentClient() instead of waiting via currentClientOrRefresh() like Do, DoWithTimeout, and Pipeline do. In that startup/race window it immediately returns ErrNoLeaderBackend, records the secondary write as failed, and never replays it after discovery, so primary-accepted traffic can be missing from ElasticKV. Use the refresh-aware lookup here as well.
Useful? React with 👍 / 👎.
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 (ddfffb7):
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. |
|
Evidence for current HEAD ddfffb7: Addressed the latest Redis proxy leader-aware backend finding:
Caller audit:
Validation:
The latest review thread is no longer active after the push. Current-head CI is running. @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
|
CI follow-up addressed on latest HEAD 40370e4. Root cause:
Change made:
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 (40370e4):
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
adapter/redis_lua_context.go (1)
3883-3883: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
//nolint:gosecの使用はガイドライン上非推奨です。
seqInTxnはdeltas（redisPairWidth上限）のループインデックスでありuint32へのナローイングは実質的に安全ですが、コーディングガイドラインでは//nolint抑制ではなくリファクタリングを優先することが求められています。境界チェック付きの変換ヘルパーへの置き換えを検討してください。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 `@adapter/redis_lua_context.go` at line 3883, Remove the //nolint:gosec suppression from the ListMetaDeltaKey call and replace the direct uint32(seqInTxn) conversion with the project’s checked integer-conversion helper. Preserve the existing bounded-index behavior and ensure the conversion handles any out-of-range value according to the helper’s established error or fallback contract.Sources: Coding guidelines, Linters/SAST tools
🤖 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/dynamodb_test.go`:
- Around line 272-280: Configure an individual timeout for each HTTP attempt in
the health-check request flow around http.NewRequestWithContext and
http.DefaultClient.Do. Use a timeout-bearing context or HTTP client, and treat
timeout errors as a false health-check result so the Eventually condition can
continue without blocking.
- Around line 278-285: Replace all three require.NoError calls within the
Eventually condition in the test around the HTTP request and response body
reads. Return false from the condition when an error occurs, or migrate the
assertion flow to require.EventuallyWithT and use its CollectT, so transient
request and read errors trigger retries instead of FailNow terminating the
condition goroutine.
---
Nitpick comments:
In `@adapter/redis_lua_context.go`:
- Line 3883: Remove the //nolint:gosec suppression from the ListMetaDeltaKey
call and replace the direct uint32(seqInTxn) conversion with the project’s
checked integer-conversion helper. Preserve the existing bounded-index behavior
and ensure the conversion handles any out-of-range value according to the
helper’s established error or fallback contract.
🪄 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: 3bdd833c-5201-4786-b4ff-88448ec88d00
📒 Files selected for processing (25)
adapter/dynamodb_test.goadapter/redis.goadapter/redis_command_specs.goadapter/redis_lua_context.goadapter/redis_peer_limiter_test.goadapter/redis_retry_test.goadapter/redis_workpool.goadapter/redis_workpool_test.goadapter/redis_zset_cmds.gocmd/redis-proxy/main.gocmd/redis-proxy/main_test.godocs/redis-proxy-deployment.mdkv/sharded_coordinator.gomain.gomonitoring/grafana/dashboards/elastickv-raft-status.jsonmonitoring/hlc.goproxy/backend.goproxy/blocking.goproxy/command.goproxy/dualwrite.goproxy/leader_aware_backend.goproxy/leader_aware_backend_test.goproxy/proxy.goproxy/proxy_test.goproxy/pubsub.go
🚧 Files skipped from review as they are similar to previous changes (5)
- adapter/redis_peer_limiter_test.go
- kv/sharded_coordinator.go
- main.go
- monitoring/hlc.go
- cmd/redis-proxy/main_test.go
|
Codex Review: Didn't find any major issues. Breezy! 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". |
|
Evidence for current HEAD cf09c1b: Addressed the latest DynamoDB health-check test findings:
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 (cf09c1b):
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: cf09c1bd44
ℹ️ 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".
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 (424e35d):
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-head review items in 424e35d.
Validation:
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. Keep them coming! 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
Root cause
When multiple redis-proxy replicas or dedicated PubSub connections share one client IP, the old server-side per-peer cap could be exhausted. ElasticKV then rejected new Redis connections, which surfaced to the proxy/client side as connection resets and async secondary drops under load.
Validation
go test ./proxy ./cmd/redis-proxy -count=1go test ./adapter -run 'TestRedisPeerLimiter|TestRedisLeaderClientPool|TestRedisBlockingLeaderClientUsesDedicatedBudgetedPool' -count=1go test ./... -run '^$'golangci-lint run ./proxy ./cmd/redis-proxy ./adapter --timeout=5mSummary by CodeRabbit
secondary-script-timeout（Luaスクリプト用）を追加