fix(conn): conn to have state machine #3559

ndyakov · 2025-10-24T10:10:31Z

Add connection state tracking instead of multiple flags
Resolve race conditions on handoff
Improve pool performance

This PR fixes issues related to us having multiple different atomic flags in the pool connections for handoff, usability, etc. Since I tried to slightly changed the get and set operations of the pool, this correctness (which negatively impacted the performance in the first place) does not have real world drawbacks and improves developer experience.

Also fixes:

…ne-conn

Copilot

Pull Request Overview

This PR refactors connection management to use a state machine architecture, replacing the previous boolean flags (usable, Inited, used) with a formal state machine that provides better concurrency control and clearer state transitions.

Key Changes:

Introduced a new ConnStateMachine with explicit states: CREATED → INITIALIZING → IDLE ⇄ IN_USE, with UNUSABLE for handoff/reauth operations
Replaced atomic boolean flags with atomic state transitions using CAS operations and FIFO waiting queues
Updated connection initialization logic to handle concurrent initialization attempts properly

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`internal/pool/conn_state.go`	New file implementing the connection state machine with FIFO waiting queue
`internal/pool/conn_state_test.go`	Comprehensive tests for state machine transitions and concurrency
`internal/pool/conn.go`	Refactored connection to use state machine instead of boolean flags
`internal/pool/pool.go`	Updated pool operations to use state machine transitions
`internal/pool/pool_single.go`	Added comments explaining SingleConnPool's non-thread-safe nature
`redis.go`	Refactored `initConn` to handle state transitions and concurrent initialization
`maintnotifications/pool_hook.go`	Updated to check handoff state before usability
`maintnotifications/pool_hook_test.go`	Updated tests to use state machine API
`maintnotifications/handoff_worker.go`	Added call to clear handoff state before closing
`maintnotifications/errors.go`	Added new error type and documentation comments
`internal/auth/streaming/pool_hook.go`	Simplified reauth logic using state machine's AwaitAndTransition
`internal/auth/streaming/pool_hook_state_test.go`	New tests verifying reauth respects state machine

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

redis.go

internal/pool/conn.go

internal/pool/conn_state.go

internal/pool/conn.go

Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

maintnotifications/errors.go

internal/pool/conn.go

internal/pool/conn_state.go

internal/pool/pool.go

internal/pool/conn_state_test.go

Co-authored-by: Copilot <[email protected]>

- this way a waiter that timesout at the same time a releaser is releasing, won't throw token. the releaser will fail to notify and will pick another waiter. this hybrid approach should be faster than channels and maintains FIFO

ofekshenawa

LGTM

htemelski-redis

Looks good, 2 minor comments

internal/pool/conn.go

Copilot

Pull Request Overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

redis.go

internal/pool/conn.go

internal/pool/pool.go

redis.go

internal/pool/conn.go

maintnotifications/handoff_worker.go

internal/pool/conn.go

redis.go

ndyakov added 4 commits October 23, 2025 14:05

wip

27591cd

wip, used and unusable states

606264e

Merge remote-tracking branch 'origin/master' into ndyakov/state-machi…

0a75466

…ne-conn

polish state machine

5721512

ndyakov requested a review from htemelski-redis October 24, 2025 10:10

ndyakov mentioned this pull request Oct 24, 2025

fix(init): conn init should be thread safe #3555

Closed

ndyakov added 4 commits October 24, 2025 14:13

correct handling OnPut

663a60e

better errors for tests, hook should work now

7526e67

fix linter

92433e6

improve reauth state management. fix tests

21bd243

ndyakov requested a review from Copilot October 24, 2025 12:07

Copilot AI reviewed Oct 24, 2025

View reviewed changes

ndyakov and others added 11 commits October 24, 2025 15:20

Update internal/pool/conn.go

3f29463

Co-authored-by: Copilot <[email protected]>

Update internal/pool/conn.go

de2f8ba

Co-authored-by: Copilot <[email protected]>

better timeouts

94fa920

empty endpoint handoff case

cfcf37d

fix handoff state when queued for handoff

3a53e1b

try to detect the deadlock

c4ed467

try to detect the deadlock x2

9ad6288

delete should be called

03b0003

improve tests

84e856e

fix mark on uninitialized connection

a2c7a25

Merge branch 'master' into ndyakov/state-machine-conn

23d0e0f

ndyakov requested a review from Copilot October 25, 2025 00:33

Copilot AI reviewed Oct 25, 2025

View reviewed changes

ndyakov and others added 5 commits October 25, 2025 21:44

Update internal/pool/conn_state_test.go

ffbe1e5

Co-authored-by: Copilot <[email protected]>

Update internal/pool/conn_state_test.go

65a6ece

Co-authored-by: Copilot <[email protected]>

Update internal/pool/pool.go

0964dcc

Co-authored-by: Copilot <[email protected]>

Update internal/pool/conn_state.go

bc42307

Co-authored-by: Copilot <[email protected]>

Update internal/pool/conn.go

33696fb

Co-authored-by: Copilot <[email protected]>

ndyakov added 6 commits November 10, 2025 03:21

fix fast semaphore that could have been starved

1dfa805

try to fix the semaphore

e418737

should properly notify the waiters

dc319c0

- this way a waiter that timesout at the same time a releaser is releasing, won't throw token. the releaser will fail to notify and will pick another waiter. this hybrid approach should be faster than channels and maintains FIFO

waiter may double-release (if closed/times out)

3fd7e08

priority of operations

5b43c3c

use simple approach of fifo waiters

dd630c7

This comment was marked as off-topic.

Sign in to view

ndyakov and others added 5 commits November 10, 2025 16:05

use simple channel based semaphores

ac84940

address linter and tests

08d9744

remove unused benchs

26ee59c

change log message

d6159b9

Merge branch 'master' into ndyakov/state-machine-conn

82dcbab

ofekshenawa previously approved these changes Nov 11, 2025

View reviewed changes

Merge branch 'master' into ndyakov/state-machine-conn

56e3ae6

htemelski-redis previously approved these changes Nov 11, 2025

View reviewed changes

internal/pool/conn.go Show resolved Hide resolved

internal/pool/conn.go Outdated Show resolved Hide resolved

address pr comments

9e24679

ndyakov dismissed stale reviews from htemelski-redis and ofekshenawa via 9e24679 November 11, 2025 11:25

ndyakov requested a review from Copilot November 11, 2025 12:11

htemelski-redis previously approved these changes Nov 11, 2025

View reviewed changes

Copilot started reviewing on behalf of ndyakov November 11, 2025 12:11 View session

Copilot finished reviewing on behalf of ndyakov November 11, 2025 12:14

Copilot AI reviewed Nov 11, 2025

View reviewed changes

address pr comments

d84b34a

ndyakov dismissed htemelski-redis’s stale review via d84b34a November 11, 2025 13:37

fix data race

2d5a75b

htemelski-redis approved these changes Nov 11, 2025

View reviewed changes

Merge branch 'master' into ndyakov/state-machine-conn

154a6ee

ndyakov merged commit 042610b into master Nov 11, 2025
22 checks passed

ndyakov deleted the ndyakov/state-machine-conn branch November 11, 2025 15:38

fix(conn): conn to have state machine #3559

fix(conn): conn to have state machine #3559

Conversation

ndyakov commented Oct 24, 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!

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!

Uh oh!

Uh oh!

This comment was marked as off-topic.

Uh oh!

ofekshenawa left a comment

Choose a reason for hiding this comment

Uh oh!

htemelski-redis left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ndyakov commented Oct 24, 2025 •

edited

Loading