client, pkg: handle canceled contexts in select paths#10930
Conversation
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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:
📝 WalkthroughWalkthroughAdds explicit ChangesCanceled-context hardening across select-based paths
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
the
size/L
![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 `@client/servicediscovery/service_discovery.go`:
- Around line 933-937: The context cancellation guard using ctx.Err() is being
checked too late in both RPC helper functions. The ctx.Err() check in the
function containing GetOrCreateGRPCConn and the similar check around line 974
need to be moved to execute before the connection is acquired, not after.
Restructure both functions to check if the context is already canceled (using
ctx.Err()) at the beginning, before calling GetOrCreateGRPCConn, so that
canceled contexts exit early without unnecessarily initializing connection
state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10fd23b4-b903-47a6-b661-28fdeac1a08f
📒 Files selected for processing (12)
client/clients/router/client.goclient/pkg/batch/batch_controller.goclient/pkg/batch/batch_controller_test.goclient/pkg/deadline/watcher.goclient/resource_manager_client.goclient/servicediscovery/service_discovery.gopkg/ratelimit/concurrency_limiter.gopkg/ratelimit/concurrency_limiter_test.gopkg/ratelimit/runner.gopkg/ratelimit/runner_test.gopkg/utils/syncutil/ordered_single_flight.gopkg/utils/syncutil/ordered_single_flight_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10930 +/- ##
==========================================
- Coverage 79.15% 79.15% -0.01%
==========================================
Files 540 540
Lines 74801 74894 +93
==========================================
+ Hits 59212 59282 +70
- Misses 11405 11430 +25
+ Partials 4184 4182 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/ratelimit/concurrency_limiter_test.go (1)
119-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest is correct, but only covers a single waiter.
This exercises the
<-ctx.Done()path with one blocked waiter. It does not cover the multi-waiter case where one canceled waiter consumes aReleaseTokenwake-up via the<-l.queuebranch (the lost-wakeup issue raised inconcurrency_limiter.go). A test with two waiters on independent contexts—cancel one, expect the other to still acquire after a release—would guard that path.Want me to draft a two-waiter cancellation test that asserts the surviving waiter still acquires the freed token?
🤖 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 `@pkg/ratelimit/concurrency_limiter_test.go` around lines 119 - 155, The test TestConcurrencyLimiterDoesNotReuseStaleQueuedToken only covers a single waiter scenario and does not test the multi-waiter case where a canceled waiter might consume a wake-up intended for another waiter via the queue channel, potentially exposing a lost-wakeup issue. Create a new test with two concurrent goroutines both calling AcquireToken with independent contexts on the same limited concurrency limiter, cancel the first waiter's context, then release a token and verify that the second waiter (not the canceled one) successfully acquires the freed token.
🤖 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 `@pkg/ratelimit/concurrency_limiter.go`:
- Around line 36-37: The NewConcurrencyLimiter function accepts a uint64 limit
parameter but implicitly converts it to an int when creating the channel buffer
with make(chan struct{}, limit). If the limit exceeds math.MaxInt64, this will
cause a runtime panic. Add a bounds validation check before the channel creation
in NewConcurrencyLimiter to ensure the limit does not exceed math.MaxInt64. You
can either return an error (if the function signature is updated), use a
sensible default value, or explicitly return nil with clear error messaging to
prevent invalid inputs from causing panics.
- Around line 136-149: In the `<-l.queue` case branch, when a waiter receives a
wake-up signal but finds its context is canceled (ctx.Err() is non-nil), the
signal is consumed and lost without being forwarded. This causes other waiting
goroutines to starve. After decrementing l.waiting and unlocking the mutex in
the cancellation error return path, send a signal back to the l.queue channel to
forward the consumed wake-up to the next waiter before returning the error.
---
Nitpick comments:
In `@pkg/ratelimit/concurrency_limiter_test.go`:
- Around line 119-155: The test
TestConcurrencyLimiterDoesNotReuseStaleQueuedToken only covers a single waiter
scenario and does not test the multi-waiter case where a canceled waiter might
consume a wake-up intended for another waiter via the queue channel, potentially
exposing a lost-wakeup issue. Create a new test with two concurrent goroutines
both calling AcquireToken with independent contexts on the same limited
concurrency limiter, cancel the first waiter's context, then release a token and
verify that the second waiter (not the canceled one) successfully acquires the
freed token.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 888aed12-46b3-45a4-b8b8-9294683c0f81
📒 Files selected for processing (2)
pkg/ratelimit/concurrency_limiter.gopkg/ratelimit/concurrency_limiter_test.go
| func NewConcurrencyLimiter(limit uint64) *ConcurrencyLimiter { | ||
| return &ConcurrencyLimiter{limit: limit, queue: make(chan *TaskToken, limit)} | ||
| return &ConcurrencyLimiter{limit: limit, queue: make(chan struct{}, limit)} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the unlimit constant and any construction with it.
rg -nP --type=go '\bunlimit\b' pkg/ratelimit
rg -nP --type=go -C2 'NewConcurrencyLimiter\s*\(' -g '!**/*_test.go'Repository: tikv/pd
Length of output: 601
🏁 Script executed:
cat -n pkg/ratelimit/concurrency_limiter.goRepository: tikv/pd
Length of output: 5341
🏁 Script executed:
rg -nP --type=go 'NewConcurrencyLimiter\s*\(' -A 2Repository: tikv/pd
Length of output: 145
🏁 Script executed:
rg -nP --type=go 'NewConcurrencyLimiter' pkg/ratelimit --glob='!*_test.go'Repository: tikv/pd
Length of output: 564
🏁 Script executed:
rg -nP --type=go 'NewConcurrencyLimiter' --glob='!*_test.go' -A 1Repository: tikv/pd
Length of output: 145
🏁 Script executed:
cat -n pkg/ratelimit/limiter.go | head -110 | tail -80Repository: tikv/pd
Length of output: 2541
🏁 Script executed:
rg -nP --type=go 'NewConcurrencyLimiter\(limit\)' -B 10 -A 2Repository: tikv/pd
Length of output: 145
🏁 Script executed:
rg -nP --type=go 'ConcurrencyLimit' pkg/ratelimit -B 2 -A 2Repository: tikv/pd
Length of output: 20320
🏁 Script executed:
rg -nP --type=go 'math.MaxInt' pkg/ratelimitRepository: tikv/pd
Length of output: 145
Add bounds validation to prevent panic from oversized channel buffer.
The concern is valid: make(chan struct{}, limit) implicitly converts the uint64 parameter to int. If limit exceeds math.MaxInt64, the conversion panics with "makechan: size out of range". While current code only uses small values (0–100 in tests), there's no input validation preventing a caller from passing a uint64 value > math.MaxInt64 via DimensionConfig.ConcurrencyLimit.
Recommend adding a bounds check: either validate limit <= math.MaxInt64 before the make() call, or change the parameter type to int64 with appropriate validation at the public API boundary.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 36-36: : # github.com/tikv/pd/pkg/dashboard/uiserver
pkg/dashboard/uiserver/embedded_assets_rewriter.go:36:26: undefined: assets
pkg/dashboard/uiserver/embedded_assets_rewriter.go:37:13: undefined: vfsgen۰FS
pkg/dashboard/uiserver/embedded_assets_rewriter.go:39:15: undefined: vfsgen۰CompressedFileInfo
pkg/dashboard/uiserver/embedded_assets_rewriter.go:47:9: undefined: assets
(typecheck)
🤖 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 `@pkg/ratelimit/concurrency_limiter.go` around lines 36 - 37, The
NewConcurrencyLimiter function accepts a uint64 limit parameter but implicitly
converts it to an int when creating the channel buffer with make(chan struct{},
limit). If the limit exceeds math.MaxInt64, this will cause a runtime panic. Add
a bounds validation check before the channel creation in NewConcurrencyLimiter
to ensure the limit does not exceed math.MaxInt64. You can either return an
error (if the function signature is updated), use a sensible default value, or
explicitly return nil with clear error messaging to prevent invalid inputs from
causing panics.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx
force-pushed
the
fix-select-cancel-races
branch
from
07bce4a to
bf704ab
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/ratelimit/concurrency_limiter_test.go (2)
190-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDirect internal state manipulation couples test to implementation details.
The test directly manipulates the limiter's internal state (setting
token.released, modifyinglimiter.current, sending tolimiter.queue) instead of using the publicReleaseTokenAPI. While this may be intentional to create a specific race scenario for testing the wake-up forwarding logic, it tightly couples the test to the internal implementation and makes it brittle.If the internal queue structure or release mechanism changes (e.g., different wake-up signaling), this test will need updates even if the public behavior remains correct.
🤖 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 `@pkg/ratelimit/concurrency_limiter_test.go` around lines 190 - 195, The test is directly manipulating the limiter's internal state by locking the mutex, setting token.released, modifying limiter.current, and sending to limiter.queue instead of using the public API. Replace this direct internal state manipulation block with a call to the public ReleaseToken method on the token object. This will decouple the test from implementation details and make it more maintainable if the internal wake-up signaling mechanism changes.
189-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment overstates ordering guarantees in concurrent scenario.
The comment claims to "force the first waiter to consume the wake-up before it observes cancellation," but after sending the wake-up (line 193) and canceling the context (line 194), both
l.queueandctx.Done()are ready in the select statement. Go's select will pseudo-randomly choose between the two cases, so the ordering is not actually forced.However, the test should pass regardless because both paths produce correct behavior:
- If
ctx.Done()is selected first: wake-up remains in queue for survivingWaiter- If
l.queueis selected first: canceledWaiter consumes wake-up, detects cancellation, and forwards a new wake-up for survivingWaiterConsider updating the comment to reflect that this creates a race scenario rather than forcing a specific ordering, or restructure to deterministically test the forwarding path if that's the intent.
🤖 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 `@pkg/ratelimit/concurrency_limiter_test.go` around lines 189 - 195, The comment at the beginning of the test block claims to force a specific ordering where the first waiter consumes the wake-up before observing cancellation, but this overstates what actually happens. After sending a wake-up on l.queue and then calling cancelCanceledWaiter() to trigger context cancellation, both l.queue and ctx.Done() are simultaneously ready, so Go's select statement will pseudo-randomly choose between them rather than guaranteeing the wake-up is consumed first. Update the comment to accurately reflect that this creates a race scenario between the queue and context cancellation cases, or alternatively restructure the test to deterministically test the wake-up forwarding behavior by ensuring proper ordering between the queue send and context cancellation trigger.
🤖 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 `@pkg/ratelimit/concurrency_limiter_test.go`:
- Around line 190-195: The test is directly manipulating the limiter's internal
state by locking the mutex, setting token.released, modifying limiter.current,
and sending to limiter.queue instead of using the public API. Replace this
direct internal state manipulation block with a call to the public ReleaseToken
method on the token object. This will decouple the test from implementation
details and make it more maintainable if the internal wake-up signaling
mechanism changes.
- Around line 189-195: The comment at the beginning of the test block claims to
force a specific ordering where the first waiter consumes the wake-up before
observing cancellation, but this overstates what actually happens. After sending
a wake-up on l.queue and then calling cancelCanceledWaiter() to trigger context
cancellation, both l.queue and ctx.Done() are simultaneously ready, so Go's
select statement will pseudo-randomly choose between them rather than
guaranteeing the wake-up is consumed first. Update the comment to accurately
reflect that this creates a race scenario between the queue and context
cancellation cases, or alternatively restructure the test to deterministically
test the wake-up forwarding behavior by ensuring proper ordering between the
queue send and context cancellation trigger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03a62b96-944c-466e-992a-353c51c39e4e
📒 Files selected for processing (2)
pkg/ratelimit/concurrency_limiter.gopkg/ratelimit/concurrency_limiter_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/ratelimit/concurrency_limiter.go
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
@rleungx: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| } | ||
| // Initialize the deadline. | ||
| timer := timerutil.GlobalTimerPool.Get(timeout) | ||
| d := &deadline{ |
There was a problem hiding this comment.
Does it always need to close the deadline? If yes, pls add a defer func to close it
2 participants
What problem does this PR solve?
Issue Number: Close #10929
What is changed and how does it work?
Check List
Tests
Release note
Summary by CodeRabbit
Bug Fixes
Tests