Conversation
Start the trace timeout before semaphore acquisition and make the semaphore wait context-aware so queued trace requests fail on timeout or cancellation instead of hanging indefinitely. Made-with: Cursor
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3026 +/- ##
==========================================
+ Coverage 58.30% 58.41% +0.11%
==========================================
Files 2079 2110 +31
Lines 171705 174882 +3177
==========================================
+ Hits 100105 102165 +2060
- Misses 62665 63696 +1031
- Partials 8935 9021 +86
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| return func() { <-api.traceCallSemaphore } | ||
| select { | ||
| case api.traceCallSemaphore <- struct{}{}: | ||
| return func() { <-api.traceCallSemaphore }, nil |
There was a problem hiding this comment.
We should check if the context is not cancelled again here before returning. It is possible that we end up here after context cancellation since there is no guaranteed ordering for select statements in go.
There was a problem hiding this comment.
Updated, added a second context check after acquiring the slot so canceled requests do not slip through that race
There was a problem hiding this comment.
@Kbhat1 That change works but I think a simple if statement to check of ctx.Err is nil would be better. something like this:
if ctx.Err() != nil {
<-api.traceCallSemaphore
return func() {}, ctx.Err()
}
return func() { <-api.traceCallSemaphore }, nil
Release a trace semaphore slot if context cancellation wins the race with acquisition so timed out requests do not briefly consume a trace worker. Made-with: Cursor
Reject trace requests immediately when all trace slots are occupied while still releasing the slot if cancellation wins the acquisition race. Made-with: Cursor
|
/backport |
## Describe your changes and provide context - Make trace RPC timeout apply to semaphore wait as well as trace execution - Prevent queued trace requests from hanging indefinitely by making semaphore acquisition context-aware. - `Add regression tests covering timeout while waiting for a slot and proper semaphore cleanup. ## Testing performed to validate your change - Verifying on node (cherry picked from commit b424748)
|
Successfully created backport PR for |
5 participants
Describe your changes and provide context
Testing performed to validate your change