fix(pool): Pool ReAuth should not interfere with handoff by ndyakov · Pull Request #3547 · redis/go-redis

ndyakov · 2025-10-14T18:02:55Z

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.

Copilot

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 existing Usable 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.}

Copilot

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 <175728472+Copilot@users.noreply.github.com>

Copilot

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 <175728472+Copilot@users.noreply.github.com>

Copilot

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 <175728472+Copilot@users.noreply.github.com>

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 <1547186+ndyakov@users.noreply.github.com>

Copilot

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.}

jit-ci

❌ 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

jit-ci

❌ 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.

jit-ci

❌ 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.

Copilot

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.}

htemelski-oss

Overall looks good.
Just one clarifying question

ndyakov force-pushed the ndyakov/pool-reauth branch from ef20907 to a65a724 Compare October 14, 2025 18:04

fix(pool): wip, pool reauth should not interfere with handoff

5fe0bfa

ndyakov force-pushed the ndyakov/pool-reauth branch from a65a724 to 5fe0bfa Compare October 14, 2025 18:06

ndyakov added 2 commits October 14, 2025 21:18

fix credListeners map

d39da69

fix race in tests

8a629fb

ndyakov force-pushed the ndyakov/pool-reauth branch from c9e9e6c to 8a629fb Compare October 14, 2025 19:45

ndyakov and others added 8 commits October 14, 2025 22:57

Merge branch 'master' into ndyakov/pool-reauth

6c54ab5

better conn usable timeout

90bfdb3

add design decision comment

07283ec

few small improvements

1bbf2e6

update marked as queued

1428068

add Used to clarify the state of the conn

e7dc339

rename test

77c0c73

fix(test): fix flaky test

011ef96

ndyakov requested review from Copilot and htemelski-oss October 15, 2025 15:57

Copilot AI reviewed Oct 15, 2025

View reviewed changes

Comment thread redis.go Outdated

Comment thread redis.go Outdated

Comment thread internal/pool/conn.go Outdated

Comment thread internal/auth/conn_reauth_credentials_listener.go Outdated

Comment thread internal/pool/buffer_size_test.go Outdated

ndyakov added the bug label Oct 15, 2025

ndyakov added 2 commits October 15, 2025 19:14

lock inside the listeners collection

e03396e

address pr comments

391b6c5

ndyakov requested a review from Copilot October 15, 2025 16:23

Copilot AI reviewed Oct 15, 2025

View reviewed changes

Comment thread internal/auth/cred_listeners.go Outdated

Comment thread internal/pool/conn.go

Comment thread internal/auth/conn_reauth_credentials_listener.go Outdated

Comment thread internal/auth/conn_reauth_credentials_listener.go Outdated

Update internal/auth/cred_listeners.go

6ad9a67

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

ndyakov requested a review from Copilot October 15, 2025 16:28

Copilot AI reviewed Oct 15, 2025

View reviewed changes

Comment thread internal/pool/buffer_size_test.go Outdated

Update internal/pool/buffer_size_test.go

0c4f8fb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

ndyakov force-pushed the ndyakov/pool-reauth branch from 5f8a0b9 to 0b2baa1 Compare October 17, 2025 09:39

wip refactor entraid

acb55d8

ndyakov force-pushed the ndyakov/pool-reauth branch from 0b2baa1 to acb55d8 Compare October 17, 2025 09:43

fix maintnotif pool hook

d74671b

ndyakov requested a review from Copilot October 21, 2025 08:27

Merge branch 'master' into ndyakov/pool-reauth

494a89b

Copilot AI reviewed Oct 21, 2025

View reviewed changes

Comment thread internal/pool/conn.go Outdated

Comment thread internal/pool/buffer_size_test.go

htemelski-oss previously approved these changes Oct 21, 2025

View reviewed changes

Update internal/pool/conn.go

3211b13

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

ndyakov dismissed htemelski-oss’s stale review via 3211b13 October 21, 2025 13:10

boekkooi-impossiblecloud and others added 5 commits October 21, 2025 16:15

chore(docs): explain why MaxRetries is disabled for ClusterClient (#3551

8ca7979

) Co-authored-by: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com>

exponential backoff

ce49979

address pr comments

67daad7

Merge remote-tracking branch 'origin/master' into ndyakov/pool-reauth

528f2e9

ndyakov requested a review from Copilot October 21, 2025 13:18

Copilot AI reviewed Oct 21, 2025

View reviewed changes

Comment thread internal/pool/pool_single.go

Comment thread internal/pool/pool_single.go Outdated

Comment thread internal/auth/streaming/pool_hook.go

Comment thread redis.go Outdated

jit-ci Bot reviewed Oct 21, 2025

View reviewed changes

Comment thread internal/auth/streaming/pool_hook.go Outdated

Comment thread internal/auth/streaming/pool_hook.go

Comment thread internal/auth/streaming/pool_hook.go

Comment thread internal/auth/streaming/pool_hook.go

address pr comments

1ee5293

ndyakov force-pushed the ndyakov/pool-reauth branch from d99ab6b to 1ee5293 Compare October 21, 2025 15:05

jit-ci Bot reviewed Oct 21, 2025

View reviewed changes

Comment thread internal/auth/streaming/pool_hook.go

Comment thread internal/auth/streaming/pool_hook.go

jit-ci Bot reviewed Oct 21, 2025

View reviewed changes

Comment thread internal/auth/streaming/pool_hook.go

Comment thread internal/auth/streaming/pool_hook.go

Comment thread internal/auth/streaming/pool_hook.go

Comment thread internal/auth/streaming/pool_hook.go

remove rlock

44917ef

ndyakov self-assigned this Oct 21, 2025

add some comments

f0fbea6

ndyakov requested review from Copilot and htemelski-oss October 21, 2025 15:27

ndyakov changed the title ~~fix(pool): wip, pool reauth should not interfere with handoff~~ fix(pool): Pool ReAuth should not interfere with handoff Oct 21, 2025

Copilot AI reviewed Oct 21, 2025

View reviewed changes

Comment thread internal/auth/streaming/pool_hook.go

add comments

e469358

htemelski-oss reviewed Oct 22, 2025

View reviewed changes

Comment thread internal/auth/streaming/cred_listeners.go

htemelski-oss approved these changes Oct 22, 2025

View reviewed changes

ndyakov merged commit a15e763 into master Oct 22, 2025
37 checks passed

ndyakov deleted the ndyakov/pool-reauth branch October 22, 2025 09:45

Conversation

ndyakov commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jit-ci Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jit-ci Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jit-ci Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

htemelski-oss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ndyakov commented Oct 14, 2025 •

edited

Loading