-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(pool): Pool ReAuth should not interfere with handoff #3547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ef20907
to
a65a724
Compare
a65a724
to
5fe0bfa
Compare
c9e9e6c
to
8a629fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a concurrency issue where pool re-authentication could interfere with connection handoff operations during maintenance notifications. The fix introduces connection state management using Usable
and Used
flags to coordinate between background operations (re-authentication) and handoff processes.
Key changes:
- Added connection-level credentials listener caching to prevent duplicate listeners per connection
- Introduced
Used
flag alongside existingUsable
flag to prevent concurrent command execution on connections - Modified re-authentication flow to wait for connections to be available before proceeding
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
redis.go | Added credentials listener management with caching per connection and modified re-authentication callbacks to accept connection parameter |
internal/pool/conn.go | Added Used flag and reorganized connection state fields, modified handoff marking logic to acquire connection before state changes |
internal/pool/pool.go | Updated connection usage tracking with Used flag in pool operations |
internal/pool/pool_single.go | Added Used flag management for single connection pools |
auth/conn_reauth_credentials_listener.go | New file implementing connection-aware credentials listener with timeout-based waiting for connection availability |
error.go | Added ErrConnUnusableTimeout to bad connection error handling |
redis_test.go | Added mutex protection to mock streaming provider |
sentinel_test.go | Renamed test function for clarity |
internal/pool/pool_test.go | Adjusted test expectations for dial retry behavior |
maintnotifications/handoff_worker.go | Added formatting and clarifying comments |
internal/pool/buffer_size_test.go | Updated unsafe pointer field layout to match new connection structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
5f8a0b9
to
0b2baa1
Compare
0b2baa1
to
acb55d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
In order to improve performance replace `WithAttributes` with `WithAttributeSet`. This avoids the slice allocation and copy that is done in `WithAttributes`. For more information see https://github.com/open-telemetry/opentelemetry-go/blob/v1.38.0/metric/instrument.go#L357-L376
) Co-authored-by: Nedyalko Dyakov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Jit has detected 4 important findings in this PR that you should review.
The findings are detailed below as separate comments.
It’s highly recommended that you fix these security issues before merge.
Repository Risks:
- Database Integration: Connects to a database, often involving sensitive data that must be securely managed.
- High Severity Findings: Indicates that the resource has high severity security findings that need attention.
Repository Context:
graph LR
GitHub$Repository_U23_redis/go_U2D_redis["GitHub Repository<br/>redis/go-redis"]:::GitHub$Repository
Team_U23_client_U2D_developers["Team<br/>client-developers"]:::Team
Team_U23_client_U2D_docs["Team<br/>client-docs"]:::Team
DBIntegration_U23_redis["DBIntegration<br/>redis"]:::DBIntegration
Team_U23_client_U2D_developers -- "Owns" --> GitHub$Repository_U23_redis/go_U2D_redis
Team_U23_client_U2D_docs -- "Owns" --> GitHub$Repository_U23_redis/go_U2D_redis
GitHub$Repository_U23_redis/go_U2D_redis -- "Is accessible to" --> DBIntegration_U23_redis
d99ab6b
to
1ee5293
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Jit has detected 4 important findings in this PR that you should review.
The findings are detailed as separate comments.
It’s highly recommended that you fix these security issues before merge.
Until now, you ignored/fixed 2 findings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Jit has detected 4 important findings in this PR that you should review.
The findings are detailed as separate comments.
It’s highly recommended that you fix these security issues before merge.
Until now, you ignored/fixed 6 findings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
// Release Usable and retry with exponential backoff | ||
// todo(ndyakov): think of a better way to do this without the need | ||
// to release the connection, but just wait till it is not used | ||
conn.SetUsable(true) | ||
} | ||
} | ||
if !acquired { | ||
// Exponential backoff: 10, 20, 40, 80... up to 5120 microseconds | ||
delay := baseDelay * time.Duration(1<<uint(attempt%10)) // Cap exponential growth | ||
time.Sleep(delay) | ||
attempt++ | ||
} |
Copilot
AI
Oct 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO indicates a known issue with the current connection acquisition approach. The release-and-retry pattern could lead to starvation if the connection is continuously used. Consider implementing a wait mechanism (e.g., using a condition variable or channel) to be notified when the connection becomes available instead of polling with exponential backoff.
// Release Usable and retry with exponential backoff | |
// todo(ndyakov): think of a better way to do this without the need | |
// to release the connection, but just wait till it is not used | |
conn.SetUsable(true) | |
} | |
} | |
if !acquired { | |
// Exponential backoff: 10, 20, 40, 80... up to 5120 microseconds | |
delay := baseDelay * time.Duration(1<<uint(attempt%10)) // Cap exponential growth | |
time.Sleep(delay) | |
attempt++ | |
} | |
// Release Usable and wait for notification | |
conn.SetUsable(true) | |
// Wait for the connection to become unused or timeout | |
usedWaitCh := make(chan struct{}) | |
go func() { | |
// Poll at a reasonable interval, but not busy-wait | |
ticker := time.NewTicker(100 * time.Microsecond) | |
defer ticker.Stop() | |
for { | |
if !conn.IsUsed() { | |
close(usedWaitCh) | |
return | |
} | |
select { | |
case <-ticker.C: | |
// continue | |
case <-timeout: | |
// timeout, exit | |
return | |
} | |
} | |
}() | |
select { | |
case <-usedWaitCh: | |
// Try again to acquire | |
case <-timeout: | |
// Timeout occurred, cannot acquire connection | |
err = pool.ErrConnUnusableTimeout | |
reAuthFn(err) | |
return | |
} | |
} | |
} |
Copilot uses AI. Check for mistakes.
This branch implements support for streaming credentials with automatic re-authentication of pooled Redis connections. The implementation adds a new pool hook system for background re-authentication, introduces a Used flag to prevent concurrent command execution on connections, and integrates with the existing maintenance notifications system.