When a Lua script containing a write command is executed against a
read-only replica in a Redis Cluster, the error may contain
"-READONLY You can't write against a read only replica" embedded
within a larger error message rather than starting with "READONLY ".
The existing code only checked for the "READONLY " prefix, missing
this format. This caused shouldRetry() and isReadOnlyError() to fail,
preventing automatic retry and connection pool cleanup.
The fix checks for the exact substring
"-READONLY You can't write against a read only replica" which is
specific enough to avoid false positives while catching the Lua
script error format.
Closes#2770
Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
* ring: remove external rendezvous dependency by inlining implementation
Replace usage of github.com/dgryski/go-rendezvous with an internal
implementation to eliminate the external dependency and avoid licence
propagation concerns.
The implementation preserves the exact hashing algorithm and behaviour:
- same hash combination: khash ^ nhash
- same mixing function (xorshift* variant)
- same node ordering semantics
No functional changes intended.
* Refactor TestRendezvous to remove randomString function
Replaced function with inline string generation for test cases.
* test: refactor RendezvousHash tests to use Ginkgo and Gomega
* test: improve RendezvousHash test for per-instance determinism and distribution balance
* test: enhance distribution verification in RendezvousHash tests
* Add DoRaw and DoRawWriteTo for raw RESP protocol access
* fix augment comments
* Pipeline retry fix
* Fix Missing NoRetry check in cluster and ring retry loops
* add dial retry backoff function.
* add readme
* add backoff to wordlist
* add logical consistency check
* fix(cluster): propagate DialerRetryBackoff to per-node clients
ClusterOptions.clientOptions() was not propagating DialerRetryBackoff,
causing the backoff function to be silently ignored when set on a cluster
client.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
---------
Co-authored-by: Nedyalko Dyakov <[email protected]>
Co-authored-by: Claude Sonnet 4.6 <[email protected]>
* fix: connection closure metrics and enable all metric groups by default
This commit addresses three issues in the redisotel-native metrics implementation:
1. Fix connection closure metrics not appearing in dashboards
- Root cause: CloseConn() in internal/pool/pool.go was not calling
the metricConnectionClosedCallback when closing stale connections
- Added callback invocation in CloseConn() to record 'stale' closures
2. Verify tracing and metrics compatibility
- Added TestTracingAndMetricsCompatibility to document that
redisotel (tracing via AddHook) and redisotel-native (metrics via
SetOTelRecorder) can coexist without conflicts
3. Enable all metric groups by default
- Changed default MetricGroups from connection-basic+resiliency
to MetricGroupAll for comprehensive observability out of the box
* Dial timeout is per-retry
Use dial timeout for each dial attempt, rather than
all attempts together.
* some tests
* additional tests
* use parent timeout so we can test our changes
---------
Co-authored-by: Nedyalko Dyakov <[email protected]>
* chore(go): update go version to 1.24
* update readmes and ci workflows
* update dockerfile for proxy-fi-server
* address changed test behaviour
* address changed test behaviour
* First draft. One metric - command duration.
Signed-off-by: Elena Kolevska <[email protected]>
* Adds connection state metrics
Signed-off-by: Elena Kolevska <[email protected]>
* wip
Signed-off-by: Elena Kolevska <[email protected]>
* Add phase 1 metrics
* Add client api configurations
* remove redundent code
* refactor: move otel-metrics example to separate module
* refactor: move otel-metrics example to separate module
* remove internal test
* Resolve merge conflicts with upstream/master
* Fix linting issues
* Fix pool idle connection counter bug
* Add OTel performance benchmarking suite
* Improve perf compatison script
* Add phase 2,3 metrics
* Remove local benchmark tests
* Add new fields to tests
* Change db.client.connection.count and db.client.connection.pending_request metric type from UpAndDownCounter to Async gauge.
* fix golangci lint
* Add piplined commands metrics support
* Redesign otel init method
* Address comments
* Fix golangci-lint errors
* Fix passing nil pointer
* Add metric prefix to callbacks and atomic batch registration
* Add unique name for pool.name attr
* Fix unique pool name for pubsub
* Refactor all hardcoded OTel attribute strings to use constants
* Refactor all hardcoded OTel attribute strings to use constants
* extract metric names and pool names as constants, improve error handling
---------
Signed-off-by: Elena Kolevska <[email protected]>
Co-authored-by: Elena Kolevska <[email protected]>
As of Go 1.20, this is the idiomatic way to convert from Bytes to
String and vice-versa.
This updates `go.mod` to 1.20, builds have already been running on
1.23.x and 1.24.x since #3274.
Co-authored-by: Nedyalko Dyakov <[email protected]>
* chore(go): update go version to 1.21
* chore(aggregators): make aggregators work with 1.21
* fix doctests
* address copilot comments
* use atomic bool for logic and/or aggregators
* Update .github/workflows/build.yml
Co-authored-by: ccoVeille <[email protected]>
* use stable/oldstable, 1.23 and 1.21
* fix versions in README
* add oldstable in wordlist
---------
Co-authored-by: ccoVeille <[email protected]>
* fix(pool): prevent double freeTurn in queuedNewConn
This commit fixes a critical race condition where freeTurn() could be
called twice in the connection pool's queuedNewConn flow, causing turn
counter inconsistency.
Problem:
- When a new connection creation failed in queuedNewConn, both the
defer handler and the dialing goroutine could call freeTurn()
- This led to turn counter underflow and queue length inconsistency
Solution:
- Modified putIdleConn to return a boolean indicating whether the
caller needs to call freeTurn()
- Returns true: connection was put back to pool, caller must free turn
- Returns false: connection was delivered to a waiting request,
turn will be freed by the receiving goroutine
- Updated queuedNewConn to only call freeTurn() when putIdleConn
returns true
- Improved error handling flow in the dialing goroutine
Changes:
- putIdleConn now returns bool instead of void
- Added comprehensive documentation for putIdleConn behavior
- Refactored error handling in queuedNewConn goroutine
- Updated test cases to reflect correct turn state expectations
This ensures each turn is freed exactly once, preventing resource
leaks and maintaining correct queue state.
* fix: sync double freeturn bug fix and context calculation from upstream
Synced from https://github.com/redis/go-redis/tree/ndyakov/freeturn-fix
Changes include:
- Add comprehensive tests for double freeTurn bug detection
- Improve context timeout calculation using min(remaining time, DialTimeout)
- Prevent goroutines from waiting longer than necessary
Co-authored-by: Nedyalko Dyakov <[email protected]>
Co-authored-by: Copilot <[email protected]>
---------
Co-authored-by: Nedyalko Dyakov <[email protected]>
Co-authored-by: Nedyalko Dyakov <[email protected]>
Co-authored-by: Copilot <[email protected]>
* wip
* wip, used and unusable states
* polish state machine
* correct handling OnPut
* better errors for tests, hook should work now
* fix linter
* improve reauth state management. fix tests
* Update internal/pool/conn.go
Co-authored-by: Copilot <[email protected]>
* Update internal/pool/conn.go
Co-authored-by: Copilot <[email protected]>
* better timeouts
* empty endpoint handoff case
* fix handoff state when queued for handoff
* try to detect the deadlock
* try to detect the deadlock x2
* delete should be called
* improve tests
* fix mark on uninitialized connection
* Update internal/pool/conn_state_test.go
Co-authored-by: Copilot <[email protected]>
* Update internal/pool/conn_state_test.go
Co-authored-by: Copilot <[email protected]>
* Update internal/pool/pool.go
Co-authored-by: Copilot <[email protected]>
* Update internal/pool/conn_state.go
Co-authored-by: Copilot <[email protected]>
* Update internal/pool/conn.go
Co-authored-by: Copilot <[email protected]>
* fix error from copilot
* address copilot comment
* fix(pool): pool performance (#3565)
* perf(pool): replace hookManager RWMutex with atomic.Pointer and add predefined state slices
- Replace hookManager RWMutex with atomic.Pointer for lock-free reads in hot paths
- Add predefined state slices to avoid allocations (validFromInUse, validFromCreatedOrIdle, etc.)
- Add Clone() method to PoolHookManager for atomic updates
- Update AddPoolHook/RemovePoolHook to use copy-on-write pattern
- Update all hookManager access points to use atomic Load()
Performance improvements:
- Eliminates RWMutex contention in Get/Put/Remove hot paths
- Reduces allocations by reusing predefined state slices
- Lock-free reads allow better CPU cache utilization
* perf(pool): eliminate mutex overhead in state machine hot path
The state machine was calling notifyWaiters() on EVERY Get/Put operation,
which acquired a mutex even when no waiters were present (the common case).
Fix: Use atomic waiterCount to check for waiters BEFORE acquiring mutex.
This eliminates mutex contention in the hot path (Get/Put operations).
Implementation:
- Added atomic.Int32 waiterCount field to ConnStateMachine
- Increment when adding waiter, decrement when removing
- Check waiterCount atomically before acquiring mutex in notifyWaiters()
Performance impact:
- Before: mutex lock/unlock on every Get/Put (even with no waiters)
- After: lock-free atomic check, only acquire mutex if waiters exist
- Expected improvement: ~30-50% for Get/Put operations
* perf(pool): use predefined state slices to eliminate allocations in hot path
The pool was creating new slice literals on EVERY Get/Put operation:
- popIdle(): []ConnState{StateCreated, StateIdle}
- putConn(): []ConnState{StateInUse}
- CompareAndSwapUsed(): []ConnState{StateIdle} and []ConnState{StateInUse}
- MarkUnusableForHandoff(): []ConnState{StateInUse, StateIdle, StateCreated}
These allocations were happening millions of times per second in the hot path.
Fix: Use predefined global slices defined in conn_state.go:
- validFromInUse
- validFromCreatedOrIdle
- validFromCreatedInUseOrIdle
Performance impact:
- Before: 4 slice allocations per Get/Put cycle
- After: 0 allocations (use predefined slices)
- Expected improvement: ~30-40% reduction in allocations and GC pressure
* perf(pool): optimize TryTransition to reduce atomic operations
Further optimize the hot path by:
1. Remove redundant GetState() call in the loop
2. Only check waiterCount after successful CAS (not before loop)
3. Inline the waiterCount check to avoid notifyWaiters() call overhead
This reduces atomic operations from 4-5 per Get/Put to 2-3:
- Before: GetState() + CAS + waiterCount.Load() + notifyWaiters mutex check
- After: CAS + waiterCount.Load() (only if CAS succeeds)
Performance impact:
- Eliminates 1-2 atomic operations per Get/Put
- Expected improvement: ~10-15% for Get/Put operations
* perf(pool): add fast path for Get/Put to match master performance
Introduced TryTransitionFast() for the hot path (Get/Put operations):
- Single CAS operation (same as master's atomic bool)
- No waiter notification overhead
- No loop through valid states
- No error allocation
Hot path flow:
1. popIdle(): Try IDLE → IN_USE (fast), fallback to CREATED → IN_USE
2. putConn(): Try IN_USE → IDLE (fast)
This matches master's performance while preserving state machine for:
- Background operations (handoff/reauth use UNUSABLE state)
- State validation (TryTransition still available)
- Waiter notification (AwaitAndTransition for blocking)
Performance comparison per Get/Put cycle:
- Master: 2 atomic CAS operations
- State machine (before): 5 atomic operations (2.5x slower)
- State machine (after): 2 atomic CAS operations (same as master!)
Expected improvement: Restore to baseline ~11,373 ops/sec
* combine cas
* fix linter
* try faster approach
* fast semaphore
* better inlining for hot path
* fix linter issues
* use new semaphore in auth as well
* linter should be happy now
* add comments
* Update internal/pool/conn_state.go
Co-authored-by: Copilot <[email protected]>
* address comment
* slight reordering
* try to cache time if for non-critical calculation
* fix wrong benchmark
* add concurrent test
* fix benchmark report
* add additional expect to check output
* comment and variable rename
---------
Co-authored-by: Copilot <[email protected]>
* initConn sets IDLE state
- Handle unexpected conn state changes
* fix precision of time cache and usedAt
* allow e2e tests to run longer
* Fix broken initialization of idle connections
* optimize push notif
* 100ms -> 50ms
* use correct timer for last health check
* verify pass auth on conn creation
* fix assertion
* fix unsafe test
* fix benchmark test
* improve remove conn
* re doesn't support requirepass
* wait more in e2e test
* flaky test
* add missed method in interface
* fix test assertions
* silence logs and faster hooks manager
* address linter comment
* fix flaky test
* use read instad of control
* use pool size for semsize
* CAS instead of reading the state
* preallocate errors and states
* preallocate state slices
* fix flaky test
* fix fast semaphore that could have been starved
* try to fix the semaphore
* should properly notify the waiters
- 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)
* priority of operations
* use simple approach of fifo waiters
* use simple channel based semaphores
* address linter and tests
* remove unused benchs
* change log message
* address pr comments
* address pr comments
* fix data race
---------
Co-authored-by: Copilot <[email protected]>
* async create conn
* update default values and testcase
* fix comments
* fix data race
* remove context.WithoutCancel, which is a function introduced in Go 1.21
* fix TestDialerRetryConfiguration/DefaultDialerRetries, because tryDial are likely done in async flow
* change to share failed to delivery connection to other waiting
* remove chinese comment
* fix: optimize WantConnQueue benchmarks to prevent memory exhaustion
- Fix BenchmarkWantConnQueue_Dequeue timeout issue by limiting pre-population
- Use object pooling in BenchmarkWantConnQueue_Enqueue to reduce allocations
- Optimize BenchmarkWantConnQueue_EnqueueDequeue with reusable wantConn pool
- Prevent GitHub Actions benchmark failures due to excessive memory usage
Before: BenchmarkWantConnQueue_Dequeue ran for 11+ minutes and was killed
After: All benchmarks complete in ~8 seconds with consistent performance
* format
* fix turn leaks
---------
Co-authored-by: Nedyalko Dyakov <[email protected]>
Co-authored-by: Hristo Temelski <[email protected]>
* internal/proto/peek_push_notification_test : Refactor test helpers to use fmt.Fprintf for buffers
Replaced buf.WriteString(fmt.Sprintf(...)) with fmt.Fprintf or fmt.Fprint in test helper functions for improved clarity and efficiency. This change affects push notification and RESP3 test utilities.
* peek_push_notification_test: revert prev formatting
* all: replace buf.WriteString with fmt.FprintF for consistency
---------
Co-authored-by: Nedyalko Dyakov <[email protected]>
* fix(pool): wip, pool reauth should not interfere with handoff
* fix credListeners map
* fix race in tests
* better conn usable timeout
* add design decision comment
* few small improvements
* update marked as queued
* add Used to clarify the state of the conn
* rename test
* fix(test): fix flaky test
* lock inside the listeners collection
* address pr comments
* Update internal/auth/cred_listeners.go
Co-authored-by: Copilot <[email protected]>
* Update internal/pool/buffer_size_test.go
Co-authored-by: Copilot <[email protected]>
* wip refactor entraid
* fix maintnotif pool hook
* fix mocks
* fix nil listener
* sync and async reauth based on conn lifecycle
* be able to reject connection OnGet
* pass hooks so the tests can observe reauth
* give some time for the background to execute commands
* fix tests
* only async reauth
* Update internal/pool/pool.go
Co-authored-by: Copilot <[email protected]>
* Update internal/auth/streaming/pool_hook.go
Co-authored-by: Copilot <[email protected]>
* Update internal/pool/conn.go
Co-authored-by: Copilot <[email protected]>
* chore(redisotel): use metric.WithAttributeSet to avoid copy (#3552)
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
* chore(docs): explain why MaxRetries is disabled for ClusterClient (#3551)
Co-authored-by: Nedyalko Dyakov <[email protected]>
* exponential backoff
* address pr comments
* address pr comments
* remove rlock
* add some comments
* add comments
---------
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Warnar Boekkooi <[email protected]>
Co-authored-by: Justin <[email protected]>
- Adds support for handling push notifications with RESP3.
- Using this support adds handlers for hitless upgrades.
---------
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Hristo Temelski <[email protected]>
* wip
* update documentation
* add streamingcredentialsprovider in options
* fix: put back option in pool creation
* add package level comment
* Initial re authentication implementation
Introduces the StreamingCredentialsProvider as the CredentialsProvider
with the highest priority.
TODO: needs to be tested
* Change function type name
Change CancelProviderFunc to UnsubscribeFunc
* add tests
* fix race in tests
* fix example tests
* wip, hooks refactor
* fix build
* update README.md
* update wordlist
* update README.md
* refactor(auth): early returns in cred listener
* fix(doctest): simulate some delay
* feat(conn): add close hook on conn
* fix(tests): simulate start/stop in mock credentials provider
* fix(auth): don't double close the conn
* docs(README): mark streaming credentials provider as experimental
* fix(auth): streamline auth err proccess
* fix(auth): check err on close conn
* chore(entraid): use the repo under redis org
* chore: optimize function `ReplaceSpaces`
Signed-off-by: fukua95 <[email protected]>
* trigger CI again because the bug of docker
Signed-off-by: fukua95 <[email protected]>
* trigger CI again because the bug of docker
Signed-off-by: fukua95 <[email protected]>
---------
Signed-off-by: fukua95 <[email protected]>
* chore: extract benchmark tests
* wip
* enable pubsub tests
* enable ring tests
* stop tests with build redis from source
* start all tests
* mix of makefile and action
* add sentinel configs
* fix example test
* stop debug on re
* wip
* enable gears for redis 7.2
* wip
* enable sentinel, they are expected to fail
* fix: linter configuration
* chore: update re versions
* return older redis enterprise version
* add basic codeql
* wip: increase timeout, focus only sentinel tests
* sentinels with docker network host
* enable all tests
* fix flanky test
* enable example tests
* tidy docker compose
* add debug output
* stop shutingdown masters
* don't test sentinel for re
* skip unsuported addscores
* Update README
bump go version in CI
* Update README.md
Co-authored-by: Copilot <[email protected]>
* Update CONTRIBUTING.md
add information about new test setup
---------
Co-authored-by: Copilot <[email protected]>
