Tune redis proxy connection capacity#1147
Conversation
📝 WalkthroughWalkthroughChangesリーダー検出フロー
接続上限とデプロイ設定
リプレイ再試行上限検証
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LeaderAwareRedisBackend
participant refreshLeaderNowIfDue
participant RedisSeed
Client->>LeaderAwareRedisBackend: Do / Pipeline / NewPubSub
LeaderAwareRedisBackend->>refreshLeaderNowIfDue: リーダー未確定時に同期リフレッシュ
refreshLeaderNowIfDue->>RedisSeed: INFOプローブ
RedisSeed-->>refreshLeaderNowIfDue: リーダー情報
refreshLeaderNowIfDue-->>LeaderAwareRedisBackend: リーダークライアント更新
LeaderAwareRedisBackend-->>Client: 操作結果またはシードPub/Sub
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![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: 30004dfa7d
ℹ️ 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".
| // BZPOP load from turning into a long retry backlog. | ||
| blockingReplayInitialDelay = 250 * time.Millisecond | ||
| blockingReplayNoEffectRetryWindow = 500 * time.Millisecond | ||
| blockingReplayNoEffectRetryWindow = 100 * time.Millisecond |
There was a problem hiding this comment.
When a client does ZADD followed quickly by BZPOPMIN and the ZADD secondary replay is still queued or committing for more than about 300ms, this 100ms window makes noEffectReplayRetryLimit equal 2, so the ZREM replay gives up after only two backoffs and records a permanent miss while the item can later appear on the secondary. The async queues still allow work to remain within the 5s SecondaryTimeout, so this narrows a previously recoverable 300–600ms ordering delay into a divergence for mutating blocking pops.
Useful? React with 👍 / 👎.
| const ( | ||
| redisPerPeerLimitEnv = "ELASTICKV_REDIS_PER_PEER_CONNECTIONS" | ||
| defaultRedisPerPeerConnectionCap = 8 | ||
| defaultRedisPerPeerConnectionCap = 64 |
There was a problem hiding this comment.
With this default, the per-peer limiter no longer constrains the noisy-client shape this limiter was added for: one host can open 37 heavy-command connections and all are accepted until 64, while the heavy-command worker pool is global (2*GOMAXPROCS) rather than per-peer. On production-sized nodes with fewer than 32 Ps, that single peer can again occupy every heavy-command slot and starve other clients unless operators override ELASTICKV_REDIS_PER_PEER_CONNECTIONS, so the raised cap should be scoped to proxy deployments or kept below the noisy-client range.
Useful? React with 👍 / 👎.
| const ( | ||
| defaultPoolSize = 128 | ||
| defaultElasticKVPoolSize = 4 | ||
| defaultElasticKVPoolSize = 16 |
There was a problem hiding this comment.
When the redis-proxy image is rolled before ElasticKV nodes that still run the previous 8-connection per-peer default, this new default pool can open 16 leader connections from the same proxy peer. Those older servers reject connections above 8 with ERR max connections per client exceeded, so secondary concurrency above the old cap turns into write errors during a mixed-version rollout unless the server cap is raised first or the proxy default stays within the old limit.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4ce5d038c
ℹ️ 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".
| const ( | ||
| defaultPoolSize = 128 | ||
| defaultElasticKVPoolSize = 4 | ||
| defaultElasticKVPoolSize = 8 |
There was a problem hiding this comment.
When the proxy targets ElasticKV nodes that still use the default per-peer cap of 8, this new default regular pool can grow to all 8 TCP sessions and keep them idle. Pub/Sub uses a separate dedicated go-redis connection rather than this pool, so after normal traffic has filled the pool a later SUBSCRIBE/Pub/Sub shadow connection from the same proxy IP becomes the 9th server-side connection and is rejected with ERR max connections per client exceeded. Please leave headroom under the default cap or require the server cap to be raised before increasing this default.
Useful? React with 👍 / 👎.
| -secondary-write-concurrency 2 \ | ||
| -secondary-script-concurrency 1 \ | ||
| -elastickv-pool-size 8 \ | ||
| -secondary-write-concurrency 8 \ |
There was a problem hiding this comment.
In dual-write/shadow deployments that copy this example, -secondary-write-concurrency now consumes the entire ElasticKV pool. The runtime derivation sets blocking replay concurrency to poolSize - writeConcurrency, and goBlockingReplay returns immediately when that capacity is 0, so mutating blocking commands such as BZPOPMIN/BLPOP will not be replayed to ElasticKV. Please keep the write limit below the pool size or set an explicit -secondary-blocking-replay-concurrency when documenting this migration mode.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41be10a1fd
ℹ️ 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".
| ### Recommended v1 shape | ||
|
|
||
| **Per-peer-IP connection cap, default `N=8`, env-configurable, | ||
| **Per-peer-IP connection cap, default `N=64`, env-configurable, |
There was a problem hiding this comment.
The implementation still sets defaultRedisPerPeerConnectionCap to 8, and the new limiter test in this commit also asserts the default is 8, so documenting the implemented v1 shape as default N=64 makes the design doc contradict the actual server behavior. Operators who rely on this line without setting ELASTICKV_REDIS_PER_PEER_CONNECTIONS would still have connections from the same proxy peer rejected after 8 sockets, including a 16-connection proxy pool.
Useful? React with 👍 / 👎.
|
@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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
proxy/leader_aware_backend_test.go (1)
556-599: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win初回 Pipeline と Pub/Sub の検出待機も回帰テストしてください。
このテストは
Doのみを検証しています。今回同じ経路へ切り替えたPipelineとNewPubSubについても、リーダー検出前に seed へ送信せず、検出後のリーダーを使うケースを追加してください。可能なら操作別のサブテストとしてまとめてください。As per coding guidelines, “Unit tests must be co-located with packages in
*_test.gofiles; prefer table-driven test cases.”🤖 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 `@proxy/leader_aware_backend_test.go` around lines 556 - 599, Extend TestLeaderAwareRedisBackend_InitialCommandWaitsForLeaderDiscovery with table-driven subtests covering Do, Pipeline, and NewPubSub. For each operation, verify it remains blocked before the info gate is released, does not contact the seed, then release discovery and assert the operation uses the discovered aliveLeader; preserve appropriate cleanup and result assertions for each API.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 `@docs/redis-proxy-deployment.md`:
- Line 417: Update the defaultElasticKVPoolSize constant and the ElasticKV
connection pool size documentation from 4 to 16 so the implementation,
documented default, and derived write concurrency align with the PR’s intended
default increase. If retaining 4 is intentional, instead revise the PR purpose
and test description accordingly.
In `@proxy/backend.go`:
- Around line 75-78: Update the comment describing defaultElasticKVPoolSize to
accurately state that the regular and blocking pools share ElasticKV’s per-peer
connection budget, while Pub/Sub sockets consume additional budget and may
remain counted until cleanup. Remove the implication that the default leaves
guaranteed room for Pub/Sub or other dedicated sockets, while retaining the
guidance about raising ELASTICKV_REDIS_PER_PEER_CONNECTIONS only after
configuring the cluster.
---
Nitpick comments:
In `@proxy/leader_aware_backend_test.go`:
- Around line 556-599: Extend
TestLeaderAwareRedisBackend_InitialCommandWaitsForLeaderDiscovery with
table-driven subtests covering Do, Pipeline, and NewPubSub. For each operation,
verify it remains blocked before the info gate is released, does not contact the
seed, then release discovery and assert the operation uses the discovered
aliveLeader; preserve appropriate cleanup and result assertions for each API.
🪄 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
Run ID: 8b86b033-82bf-43bb-9378-84af43d281d1
📒 Files selected for processing (8)
adapter/redis_peer_limiter_test.gocmd/redis-proxy/main_test.godocs/design/2026_04_24_implemented_workload_isolation.mddocs/redis-proxy-deployment.mdproxy/backend.goproxy/leader_aware_backend.goproxy/leader_aware_backend_test.goproxy/proxy_test.go
| |-----------|-------|-------------| | ||
| | Redis connection pool size | 128 | Default go-redis pool size for Redis | | ||
| | ElasticKV connection pool size | 4 | Default per-leader pool; keep within the server per-peer connection limit | | ||
| | ElasticKV connection pool size | 4 | Default per-leader pool; keep within the server per-peer connection limit and leave room for dedicated sockets | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
実装のデフォルト値とドキュメントをPRの目的に揃えてください。
この表はElasticKVのデフォルトpool sizeを4と記載していますが、PR目的は4から16へのデフォルト増加です。現在の proxy/backend.go の defaultElasticKVPoolSize も4のため、明示的な設定なしでは実行時のデフォルトは変わらず、派生write concurrencyも2のままです。定数とドキュメントを16へ更新するか、4が意図的ならPRの目的とテスト説明を修正してください。
🤖 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 `@docs/redis-proxy-deployment.md` at line 417, Update the
defaultElasticKVPoolSize constant and the ElasticKV connection pool size
documentation from 4 to 16 so the implementation, documented default, and
derived write concurrency align with the PR’s intended default increase. If
retaining 4 is intentional, instead revise the PR purpose and test description
accordingly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f3b80d715
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2607b32ec9
ℹ️ 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 cli != nil { | ||
| return cli | ||
| } | ||
| b.RefreshLeaderNow(ctx) |
There was a problem hiding this comment.
When INFO replication returns an empty raft_leader_redis (which the adapter emits while the Raft leader is unknown), currentClient() remains nil, so every subsequent Do/Pipeline call reaches this synchronous RefreshLeaderNow. After each failed sweep closes refreshDone, the next sequential client command immediately starts another full seed probe, bypassing refreshInterval; during startup/election/misconfigured leader maps this can turn normal traffic into repeated INFO sweeps and add probe latency to every command instead of keeping discovery on the bounded poll cadence.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/redis-proxy-deployment.md (1)
38-38: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win専用Pub/Sub分の接続予算を具体化してください。
ELASTICKV_REDIS_PER_PEER_CONNECTIONSを単にプールサイズより大きくするだけでは、専用Pub/Sub接続やクリーンアップ待ちの切り離し済みソケット分を保証できません。既定構成では command pool 64 に dedicated headroom 64 が加わるため、少なくとも「プールサイズ + 必要な専用接続数」（既定値なら128）を設定する旨を記載してください。修正例
-| `-elastickv-pool-size` | `64` | ElasticKV backend command pool size; keep the server-side `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` above this to leave room for dedicated PubSub connections | +| `-elastickv-pool-size` | `64` | ElasticKV backend command pool size; set `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` to at least this value plus the required dedicated Pub/Sub headroom (128 with the default 64-connection headroom) |同じ基準を Troubleshooting の項目にも反映してください。
Also applies to: 442-442
🤖 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 `@docs/redis-proxy-deployment.md` at line 38, Update the `-elastickv-pool-size` documentation and the corresponding Troubleshooting entry to state that `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` must be at least the command pool size plus the required dedicated Pub/Sub connection headroom, including disconnected sockets awaiting cleanup; for the default pool size of 64, document a minimum value of 128 rather than merely requiring it to be greater than the pool size.
🤖 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_peer_limiter_test.go`:
- Around line 106-107: Update the “legacy cap” case in the pool-size test to
expect normal and blocking allocations totaling no more than the limit of 8,
typically 4 each or values derived from that cap. Keep “default cap” as the
separate case validating the current default pool sizes.
In `@proxy/leader_aware_backend_test.go`:
- Around line 615-643: Ensure the INFO gate created in this test is always
released, including when assertions or t.Fatalf execute before the explicit
close(gate) call. Update the cleanup or defer logic around the gate in the test
using aliveLeader.SetInfoGate so backend.Close can complete without leaving the
fake node blocked.
---
Outside diff comments:
In `@docs/redis-proxy-deployment.md`:
- Line 38: Update the `-elastickv-pool-size` documentation and the corresponding
Troubleshooting entry to state that `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` must
be at least the command pool size plus the required dedicated Pub/Sub connection
headroom, including disconnected sockets awaiting cleanup; for the default pool
size of 64, document a minimum value of 128 rather than merely requiring it to
be greater than the pool size.
🪄 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: 317293b0-dabb-4241-862c-66f4ec820f9a
📒 Files selected for processing (6)
adapter/redis_peer_limiter_test.godocs/redis-proxy-deployment.mdproxy/backend.goproxy/leader_aware_backend.goproxy/leader_aware_backend_test.goproxy/proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- proxy/backend.go
- proxy/proxy_test.go
| {name: "legacy cap", limit: 8, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize}, | ||
| {name: "default cap", limit: defaultRedisPerPeerConnectionCap, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
「legacy cap」の期待値を上限8に合わせてください。
limit: 8 のケースでデフォルトのプールサイズを期待しているため、プール合計が8を超える構成では Line 113 の上限検証に必ず失敗します。旧上限の回帰ケースは通常4・ブロッキング4（または上限から算出した値）を期待し、現在のデフォルト値の検証は別ケースに分離してください。
修正例
- {name: "legacy cap", limit: 8, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize},
+ {name: "legacy cap", limit: 8, wantNormal: 4, wantBlocking: 4},📝 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.
| {name: "legacy cap", limit: 8, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize}, | |
| {name: "default cap", limit: defaultRedisPerPeerConnectionCap, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize}, | |
| {name: "legacy cap", limit: 8, wantNormal: 4, wantBlocking: 4}, | |
| {name: "default cap", limit: defaultRedisPerPeerConnectionCap, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize}, |
🤖 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_peer_limiter_test.go` around lines 106 - 107, Update the
“legacy cap” case in the pool-size test to expect normal and blocking
allocations totaling no more than the limit of 8, typically 4 each or values
derived from that cap. Keep “default cap” as the separate case validating the
current default pool sizes.
| gate := make(chan struct{}) | ||
| aliveLeader.SetInfoGate(gate) | ||
|
|
||
| backend := NewLeaderAwareRedisBackendWithInterval( | ||
| []string{"127.0.0.1:1", aliveLeader.addr}, | ||
| "elastickv", | ||
| DefaultBackendOptions(), | ||
| time.Hour, 200*time.Millisecond, | ||
| testLogger, | ||
| ) | ||
| t.Cleanup(func() { _ = backend.Close() }) | ||
|
|
||
| require.Empty(t, backend.CurrentLeader(), "initial commands must not target the first seed before discovery") | ||
|
|
||
| done := make(chan error, 1) | ||
| go func() { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | ||
| defer cancel() | ||
| done <- tt.run(ctx, backend) | ||
| }() | ||
|
|
||
| select { | ||
| case err := <-done: | ||
| t.Fatalf("operation returned before leader discovery completed: %v", err) | ||
| case <-time.After(50 * time.Millisecond): | ||
| } | ||
| require.Equal(t, int64(0), aliveLeader.commands.Load(), "operation must not run before discovery completes") | ||
|
|
||
| close(gate) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
失敗時にも INFO ゲートを必ず解放してください。
ゲート解放前の t.Fatalf が発生すると、backend.Close() は進行中の refresh を待ち続け、fake node 側の INFO 処理は gate 待ちのままになります。回帰を検出したテストがタイムアウトで停止します。
修正案
gate := make(chan struct{})
+ var releaseGate sync.Once
+ defer func() {
+ releaseGate.Do(func() {
+ close(gate)
+ })
+ }()
aliveLeader.SetInfoGate(gate)
...
- close(gate)
+ releaseGate.Do(func() {
+ close(gate)
+ })📝 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.
| gate := make(chan struct{}) | |
| aliveLeader.SetInfoGate(gate) | |
| backend := NewLeaderAwareRedisBackendWithInterval( | |
| []string{"127.0.0.1:1", aliveLeader.addr}, | |
| "elastickv", | |
| DefaultBackendOptions(), | |
| time.Hour, 200*time.Millisecond, | |
| testLogger, | |
| ) | |
| t.Cleanup(func() { _ = backend.Close() }) | |
| require.Empty(t, backend.CurrentLeader(), "initial commands must not target the first seed before discovery") | |
| done := make(chan error, 1) | |
| go func() { | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| done <- tt.run(ctx, backend) | |
| }() | |
| select { | |
| case err := <-done: | |
| t.Fatalf("operation returned before leader discovery completed: %v", err) | |
| case <-time.After(50 * time.Millisecond): | |
| } | |
| require.Equal(t, int64(0), aliveLeader.commands.Load(), "operation must not run before discovery completes") | |
| close(gate) | |
| gate := make(chan struct{}) | |
| var releaseGate sync.Once | |
| defer func() { | |
| releaseGate.Do(func() { | |
| close(gate) | |
| }) | |
| }() | |
| aliveLeader.SetInfoGate(gate) | |
| backend := NewLeaderAwareRedisBackendWithInterval( | |
| []string{"127.0.0.1:1", aliveLeader.addr}, | |
| "elastickv", | |
| DefaultBackendOptions(), | |
| time.Hour, 200*time.Millisecond, | |
| testLogger, | |
| ) | |
| t.Cleanup(func() { _ = backend.Close() }) | |
| require.Empty(t, backend.CurrentLeader(), "initial commands must not target the first seed before discovery") | |
| done := make(chan error, 1) | |
| go func() { | |
| ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) | |
| defer cancel() | |
| done <- tt.run(ctx, backend) | |
| }() | |
| select { | |
| case err := <-done: | |
| t.Fatalf("operation returned before leader discovery completed: %v", err) | |
| case <-time.After(50 * time.Millisecond): | |
| } | |
| require.Equal(t, int64(0), aliveLeader.commands.Load(), "operation must not run before discovery completes") | |
| releaseGate.Do(func() { | |
| close(gate) | |
| }) |
🤖 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 `@proxy/leader_aware_backend_test.go` around lines 615 - 643, Ensure the INFO
gate created in this test is always released, including when assertions or
t.Fatalf execute before the explicit close(gate) call. Update the cleanup or
defer logic around the gate in the test using aliveLeader.SetInfoGate so
backend.Close can complete without leaving the fake node blocked.
|
@codex review |
|
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". |
1 participant
Author: bootjp
Summary
Production observation
Tests
Note
Summary by CodeRabbit
機能改善
ドキュメント