feat(multidb): Introduce MultiDB client for Active-Active support - #3954
Draft
ndyakov wants to merge 23 commits into
Draft
feat(multidb): Introduce MultiDB client for Active-Active support#3954ndyakov wants to merge 23 commits into
ndyakov wants to merge 23 commits into
Conversation
Add the 'feature/**' pattern to the pull_request.branches filter of the test (build), govulncheck, codeql, and doctests workflows so that PRs targeting the multidb integration branch (feature/multidb-integration) trigger CI. golangci-lint and spellcheck already run on every PR. Part of the MultiDBClient geo-failover feature.
…er (#3835) * internal/circuitbreaker: add unified circuit breaker Add a unified circuit breaker implementation in internal/circuitbreaker, create a wrapper in internal/multidb with multidb CircuitState types, and refactor maintnotifications to embed the internal circuit breaker. - State machine (Closed -> Open -> HalfOpen -> Closed) - Lock-free atomic operations - Configurable thresholds and timeouts - State change callbacks with Reset() notification - Execute() method for wrapping function calls - MaxHalfOpenRequests for limiting half-open test requests maintnotifications.CircuitBreaker now embeds *circuitbreaker.CircuitBreaker and wires endpoint-specific logging via OnStateChange, preserving its existing public API (type alias + constants). Part of the MultiDBClient geo-failover feature (PR 1 of 11). * ci: run PR workflows for feature/** base branches Add the 'feature/**' pattern to the pull_request.branches filter of the test (build), govulncheck, codeql, and doctests workflows so that PRs targeting the multidb integration branch (feature/multidb-integration) trigger CI. golangci-lint and spellcheck already run on every PR. Part of the MultiDBClient geo-failover feature. * internal/circuitbreaker: address review comments - CheckState: guard against a zero last-failure timestamp and compare UnixNano directly, so an open circuit with no recorded failure is not treated as if it failed at the Unix epoch. - RecordSuccess: notify state-change callbacks before resetting the success/failure/request counters so callbacks observe the success count that triggered the half-open->closed transition. - maintnotifications CircuitBreaker.IsOpen: use CheckState so the open->half-open transition is honored once OpenTimeout elapses instead of reporting a stale open state. Part of the MultiDBClient geo-failover feature (PR 1 of 11). * maintnotifications: address circuit breaker review comments Tighten the circuit breaker wrapper and its handoff integration in response to PR #3835 review: - internal/circuitbreaker: notify state-change callbacks before resetting counters on the half-open -> open path so observers see the counts that triggered the transition, matching the half-open -> closed path. Add ReleaseHalfOpen to return a reserved half-open probe slot when an operation produces neither a recordable success nor failure. - maintnotifications: hold the internal breaker as an unexported field instead of embedding it, so its exported methods are not promoted onto the wrapper and callers cannot bypass the lastSuccessTime bookkeeping. Add explicit allowRequest/releaseRequest forwarding and use errors.Is when translating ErrCircuitOpen. - handoff_worker: gate handoffs with allowRequest so concurrent handoffs to a recovering endpoint stay bounded by MaxHalfOpenRequests, and release the reserved slot on initialization errors. Add a ReleaseHalfOpen unit test covering slot release, the negative-count guard, and the closed-state no-op. * reset state on transition
* internal/failuredetector: add failure detector
Add a sliding-window failure detector used by MultiDBClient to decide
when failover should be triggered.
- FailureDetector interface: RecordSuccess, RecordFailure, ShouldFailover, Reset
- Sliding time window over recent outcomes
- Context-cancellation errors are not counted as failures
- Lock-protected state, safe for concurrent use
Part of the MultiDBClient geo-failover feature (PR 2 of 11).
* ci: run PR workflows for feature/** base branches
Add the 'feature/**' pattern to the pull_request.branches filter of the
test (build), govulncheck, codeql, and doctests workflows so that PRs
targeting the multidb integration branch (feature/multidb-integration)
trigger CI. golangci-lint and spellcheck already run on every PR.
Part of the MultiDBClient geo-failover feature.
* internal/failuredetector: address review comments
- NewCommandFailureDetector: default a non-positive FailureDetectionWindow
to 2s so the window is not treated as permanently expired (which would
reset counters on every call and prevent failover). MinNumFailures and
FailureRateThreshold keep their documented zero-means-disabled semantics.
- RecordFailure: ignore a nil error so callers that forward errors
unconditionally don't accumulate phantom failures.
- Stats: apply checkWindow under the lock so callers never observe stale
counts from an elapsed window, consistent with ShouldFailover.
- Document that FailureDetectionWindow is a tumbling (not sliding) window.
- Widen the WindowReset test sleep buffer to avoid CI flakiness; add tests
for the nil-error guard and the defaulted window.
Part of the MultiDBClient geo-failover feature (PR 2 of 11).
* refactor(failuredetector): use bucketed sliding window with atomics
The previous implementation used a single time stamp plus two counters
guarded by sync.Mutex. The window was tumbling: once windowStart aged
beyond FailureDetectionWindow, both counters were zeroed in one step.
That has two failure modes:
1. Boundary cliff. A burst of failures landing in the last few ms of
a window is visible until the window flips and then all of it
disappears at once. ShouldFailover toggles around the flip, and a
ShouldFailover call right after the flip starts a fresh "0/0"
window regardless of what just happened.
2. Warmup denominator. Right after a flip the denominator is tiny,
so a single failure with FailureRateThreshold=0.5 produces a 100%
rate and triggers immediately.
Replace the implementation with a bucketed sliding window:
- Fixed ring of NumBuckets (default 10) per detector; each bucket
holds atomic.Int64 epoch and two atomic.Uint64 counters.
- RecordSuccess / RecordFailure are lock-free: compute the bucket
that owns the current nanosecond, lazily reset the slot via CAS on
the epoch when a new lap of the ring revisits it, then Add(1) to
the relevant counter. Hot path is ~67 ns/op zero-alloc under
parallel load (Apple M4 Max), most of which is time.Now().UnixNano().
- ShouldFailover / Stats sum N atomic loads (~30 ns/op). A bucket is
included iff its time slot [epoch, epoch + bucketWidth) overlaps
the trailing window (now - W, now], i.e. epoch > now - W - bucketWidth.
Outcomes age out one bucket at a time as time advances.
- The FailureDetector interface, the config field names, and the
documented "zero disables this half of the check" semantics are
preserved so the rest of the multidb stack does not need to change.
- MinNumFailures and Stats counts switch from int to uint64 to match
the atomic API and to avoid overflow on 32-bit platforms.
- Stats is now read-only; the previous implementation mutated state
(via checkWindow) on every Stats call, which made stats observers
able to trigger window flips and lose data.
- Add an injectable clock (CommandFailureDetector.now, defaulted to
time.Now) so window-expiry tests advance time deterministically
instead of sleeping.
Trade-offs documented in the source:
- A small race exists between the epoch CAS and the two counter
Stores in bucketFor: a concurrent writer can read the updated
epoch, skip the reset, and Add(1) before the Stores zero the
bucket. The increment is lost. For a failure detector deciding
"more than M failures in the last W seconds" this is in the noise:
only writes that land in the few-nanosecond window during a bucket
rotation are affected.
- Snapshot is not atomic across buckets (the N loads are sequential),
so a snapshot can interleave with concurrent writers and undercount
the true value by at most the in-flight writes. Same property as
the previous mutex-protected code.
Tests:
- Existing tests updated for the new uint64 Stats signature.
- TestCommandFailureDetector_WindowExpiry rewritten with the fake
clock; no more time.Sleep.
- TestCommandFailureDetector_SlidingWindow asserts the property that
distinguishes a sliding window from a tumbling one: outcomes age
out one bucket at a time rather than en masse.
- TestCommandFailureDetector_ConcurrentRecord drives 32 goroutines x
5000 ops each and asserts the totals are exact (no lost writes
when no bucket rotation occurs).
- TestCommandFailureDetector_ConcurrentRecordWithReaders mixes
concurrent writers and ShouldFailover readers; passes under -race.
- Three benchmarks (RecordSuccess, RecordFailure, ShouldFailover)
for future regression checks.
* refactor(failuredetector): apply defaults, add Ignore* opt-outs, fix divide-by-zero
Address review comments on PR #3836 (Cursor + Copilot reviews of
commit 53c0a4c):
1. MinNumFailures and FailureRateThreshold are now defaulted by
NewCommandFailureDetector instead of being silently treated as
"disabled". The field doc comments already promised "Default: 1000"
and "Default: 0.1", and reviewers correctly pointed out that the
constructor did not apply them. The four defaults are now extracted
as package constants (defaultMinNumFailures, ...) that both
applyDefaults and DefaultCommandFailureDetectorConfig share, so the
docs cannot drift from the code.
2. bucketWidthNano is clamped to at least one nanosecond. Previously,
a FailureDetectionWindow shorter than NumBuckets nanoseconds (e.g.
5 ns / 10 buckets) would produce bucketWidthNano=0 and panic the
first call to RecordSuccess or RecordFailure via the "%" operator.
The new clamp degrades gracefully: every record lands in the same
bucket, which is fine because the configuration itself is
pathological.
Removing the "0 = disabled" semantics loses a feature (the OR-vs-AND
switch between the two thresholds), so we replace it with an explicit,
typed escape hatch:
IgnoreMinNumFailures bool // overrides MinNumFailures
IgnoreFailureRateThreshold bool // overrides FailureRateThreshold
A paired bool reads correctly when scanning the struct, is impossible
to confuse with a numeric value, and avoids magic sentinels (-1 cannot
represent disabled on a uint64 field anyway, and Go stdlib does not use
-1 sentinels for this kind of config). When the flag is set, ShouldFailover
short-circuits the corresponding check; the "at least one failure must
be observed" rule still applies, so a freshly-constructed detector with
both flags set still returns false until something fails.
ShouldFailover loses the now-dead "threshold == 0 means disabled"
branches; the body drops from 13 lines to 11 and reads top-to-bottom.
The lost-writes race between the epoch CAS and the two Store(0) calls
in bucketFor (also flagged by Copilot) is intentionally left in place;
it is documented as accepted noise in the original commit message and
fixing it would require a sentinel-epoch spin-wait protocol that adds
complexity without a measurable benefit for a failure detector that
aggregates over thousands of events per window.
Tests:
- Remove TestCommandFailureDetector_RateOnly and _CountOnly. They
exercised the "0 = disabled" semantics that no longer exist; the
same behaviour is now covered by the Ignore* tests below.
- TestCommandFailureDetector_AppliesDefaultsForZeroFields asserts
that NewCommandFailureDetector(CommandFailureDetectorConfig{})
produces a detector whose internal config matches
DefaultCommandFailureDetectorConfig() field-for-field.
- TestCommandFailureDetector_PreservesExplicitValues asserts that
applyDefaults does not overwrite explicitly-set non-zero values.
- TestCommandFailureDetector_DoesNotPanicOnTinyWindow constructs a
detector with the exact pathological config from the reports
(FailureDetectionWindow=5ns, NumBuckets=10) and exercises both
hot-path entry points plus ShouldFailover without panic.
- TestCommandFailureDetector_IgnoreMinNumFailures /
_IgnoreFailureRateThreshold / _IgnoreBothThresholds cover the
three opt-out combinations and verify the "at least one failure"
rule still holds when both ignore flags are set.
* update comments
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* redis/internal: add geofailover OTel recorder plumbing Add the MultiDB (geo-failover) recording surface to the OTel plumbing so later PRs can emit failover/observability signals through the existing recorder indirection: - OTelRecorder (otel.go): four new RecordMultiDB* methods (Failover, ActiveDatabaseChange, CircuitStateChange, HealthCheck) plus the otelRecorderAdapter forwarders. - internal/otel.Recorder (internal/otel/metrics.go): the same four methods, matching package-level helper funcs, and no-op implementations on noopRecorder. This is purely additive and no-op safe: with no recorder registered the global noopRecorder handles the calls, so behavior is unchanged until a recorder is wired up (later PR). Part of the MultiDBClient geo-failover feature (PR 9 of 11). * redis: move MultiDB OTel methods to optional capability interface Adding the four RecordMultiDB* methods directly to the core OTelRecorder interface was a breaking change for existing implementations: the extra/redisotel-native metricsRecorder no longer satisfied OTelRecorder, so go vet (and therefore every test-redis-ce job) failed in that module. Move the MultiDB methods into a new optional OTelMultiDBRecorder capability interface, mirroring the existing OTelConnectionCounter pattern, and have otelRecorderAdapter type-assert for it before forwarding. This keeps OTelRecorder backward compatible; recorders opt in by implementing OTelMultiDBRecorder (wired up in a later PR). Part of the MultiDBClient geo-failover feature (PR 9 of 11).
* multidb: add health check implementations
Add health check implementations for MultiDBClient:
- PingHealthCheck: single PING probe with configurable probes/delay/timeout
- LagAwareHealthCheck: gates on replication lag via the Redis Enterprise REST API
- Policies (HealthyAllPolicy, HealthyMajorityPolicy, HealthyAnyPolicy) that govern
how probes WITHIN each health check are interpreted; across multiple checks the
relationship is always AND (every check must pass regardless of policy)
The policy runners execute checks concurrently with a per-check timeout and a
buffered results channel sized to the number of checks, so a hung check cannot
wedge the policy and worker goroutines never leak on early return.
Also add the MultiDBHealthCheck interface to the root redis package (the minimal
contract these checks implement: CheckHealth / CheckClusterHealth). The remaining
MultiDBClient options and interfaces follow in a later PR.
Part of the MultiDBClient geo-failover feature (PR 3 of 11).
* multidb: address health-check review comments
- NewLagAwareHealthCheck now takes typed ...LagAwareHealthCheckOption
(no more ...interface{}); add WithLagAwareHealthCheckConfig wrapper to
apply generic HealthCheckOption values (probes/delay/timeout).
- Surface TLS configuration errors via a configErr field: invalid PEM,
unreadable CA file, and bad client key pairs are recorded and cause
health checks to fail fast instead of silently misconfiguring TLS.
- Clone http.DefaultTransport (instead of &http.Transport{}) so proxy and
dial/keepalive defaults are preserved when only TLS is overridden.
- Robust host extraction via net.SplitHostPort (hostFromAddr): handles
IPv6 brackets and fails fast for unix-socket addresses.
- Trim trailing slash from baseURL to avoid double-slash REST URLs.
- runProbesMajority: add symmetric early-success exit once a strict
majority of probes have succeeded.
- Tests: assert PingHealthCheck success and skip when Redis is
unavailable (repo convention); add hostFromAddr and configErr tests.
Part of the MultiDBClient geo-failover feature (PR 3 of 11).
* multidb: address health-check review comments (round 2)
Addresses the remaining unresolved review comments on PR #3837:
- MultiDBHealthCheck.CheckHealth/CheckClusterHealth now return (bool, error)
so callers (e.g. health-check metrics) can record why a check failed.
Threaded through PingHealthCheck, LagAwareHealthCheck, the probe func type,
the three probe runners, and the test mocks. (ofekshenawa)
- Collapse the three identical policy execute() methods into a single shared
runChecks helper parameterized by the per-check runner; policies now differ
only in which runProbesX they pass in. (ofekshenawa)
- getConfig clamps invalid configurable values (Probes<=0, Timeout<=0,
Delay<0) to defaults so a misconfigured check can't trivially pass or time
out immediately. (Copilot)
- LagAware base URL uses net.JoinHostPort so IPv6 literals are bracketed
(https://[::1]:9443 rather than the malformed https://::1:9443). (Cursor)
Adds tests for IPv6 URL construction, config clamping, error propagation from
CheckHealth, and unix-socket address handling.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* update log
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* basic auth with just username
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* multidb: fix remaining health-check review gaps
- PingHealthCheck.CheckClusterHealth now fails when no shard was pinged
(empty topology) instead of returning healthy, matching
LagAwareHealthCheck which fails when the cluster has no addresses. (Cursor)
- getBDBs applies basic auth whenever a username is set, mirroring
checkLagHealth, so an intentionally empty password still sends an
Authorization header. (Copilot)
Adds a test asserting CheckClusterHealth reports false+error for an
empty/unreachable cluster.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* address pr comments
* cleanup before merge
* test(multidb): rename duplicate TLS-clone test to fix redeclaration
* address ipv6 comment
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands go-redis’ observability and resiliency primitives in support of a new MultiDB (active-active / geo-failover) client by adding health-check policies, circuit breaker and failure detector internals, and corresponding OpenTelemetry metric hooks.
Changes:
- Add
multidbhealth-check framework (policies + Ping and Redis Enterprise REST lag-aware checks) with unit tests. - Introduce reusable internal primitives for circuit breaking (
internal/circuitbreaker), multi-db circuit breaker wrapper (internal/multidb), and sliding-window failure detection (internal/failuredetector). - Extend OTel plumbing (
otel.go,internal/otel/metrics.go) with optional MultiDB metric recording and update maintnotifications to use the shared circuit breaker implementation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| otel.go | Adds an optional OTelMultiDBRecorder capability interface and adapter forwarding methods. |
| internal/otel/metrics.go | Extends the internal OTel recorder surface with MultiDB metric entry points and noop implementations. |
| multidb_healthcheck.go | Introduces the public redis.MultiDBHealthCheck interface used by MultiDB health checking. |
| multidb/healthcheck.go | Adds health check configuration, probe policies, and concurrent execution plumbing. |
| multidb/healthcheck_ping.go | Implements a PING-based health check for standalone and cluster clients. |
| multidb/healthcheck_lag_aware.go | Implements Redis Enterprise REST-based lag-aware health checking with TLS/auth options. |
| multidb/healthcheck_test.go | Adds tests for health checks, policies, and edge cases (IPv6 URL construction, config clamping, panic recovery). |
| internal/circuitbreaker/circuit_breaker.go | Adds a shared internal circuit breaker implementation with callbacks, stats, and half-open slot release. |
| internal/circuitbreaker/circuit_breaker_test.go | Adds comprehensive tests for the internal circuit breaker behavior and invariants. |
| internal/multidb/circuit_breaker.go | Adds a MultiDB-facing wrapper around the shared circuit breaker. |
| internal/multidb/circuit_breaker_test.go | Adds tests for the MultiDB circuit breaker wrapper. |
| internal/failuredetector/failure_detector.go | Adds a lock-free, bucketed sliding-window failure detector for failover decisions. |
| internal/failuredetector/failure_detector_test.go | Adds tests/benchmarks for the failure detector including window expiry and concurrency. |
| maintnotifications/circuit_breaker.go | Refactors maintnotifications circuit breaker to wrap internal/circuitbreaker and updates logging/state handling. |
| maintnotifications/circuit_breaker_test.go | Updates tests to validate behavior via public stats/state after internal refactor. |
| maintnotifications/handoff_worker.go | Gates handoff attempts using half-open slot reservation and releases slots on non-network failures. |
| .github/workflows/doctests.yaml | Expands doctest workflow PR branch patterns to include feature/**. |
Suppressed comments (1)
multidb/healthcheck_test.go:54
- Same flakiness concern as above: using a hard-coded high port for an "unreachable" cluster can be non-deterministic if something is listening there. Using 127.0.0.1:0 makes the failure deterministic.
client := redis.NewClusterClient(&redis.ClusterOptions{
Addrs: []string{"localhost:59999"},
DialTimeout: 100 * time.Millisecond,
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- NewCircuitBreaker applies the documented defaults before storing the config, so Config() reports the values the breaker actually runs with - unreachable-endpoint tests dial port 0 (deterministic failure) instead of assuming an arbitrary port is unused - WithLagAwareTLSConfig documents that a nil config clears the TLS configuration (tls.Config.Clone is nil-safe)
Member
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
multidb/healthcheck.go:132
- runChecks returns early on the first failing check, but it doesn’t cancel the shared context. That means the other concurrently-running health checks can continue doing unnecessary work (HTTP calls / PINGs) even though the final result is already known. Consider wrapping ctx with context.WithCancel and canceling it on the first false result so other checks can abort promptly.
// Every health check must pass (AND across checks).
for result := range results {
if !result {
return false
}
multidb/healthcheck_test.go:22
- This test hardcodes "localhost:6379" and ignores REDIS_PORT, which the repo uses in other plain
go testsuites to let CI/dev override the Redis port. As written, this subtest will silently skip whenever Redis is reachable on the configured port but not on 6379, reducing signal. Consider using a small helper that mirrors apTestAddr() (REDIS_PORT else :6379) and reuse it for any test that needs a live Redis connection.
client := redis.NewClient(&redis.Options{Addr: "localhost:6379"})
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 681c952ec4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- half-open probes that succeed without closing the circuit release their admission slot, so MaxHalfOpenRequests bounds concurrent probes and a value below SuccessThreshold can no longer starve recovery - lag-aware BDB matching includes the Redis port, disambiguating Enterprise databases that share a DNS name - runChecks cancels the remaining health-check workers as soon as one check fails
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b4f425b07
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Clear the stale failure counter before Closed is published and the half-open counters before HalfOpen is published (serialized by a small transition mutex), so concurrent traffic at either edge can neither re-open a just-recovered circuit off the old count nor overrun MaxHalfOpenRequests. Deep-copy RootCAs and Certificates in WithLagAwareTLSConfig, and match BDB DNS names case-insensitively.
Member
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
multidb/healthcheck_lag_aware.go:79
- WithLagAwareHTTPClient will happily accept a nil client; if that happens NewLagAwareHealthCheck will skip installing the default client and later calls will panic on h.httpClient.Do(...). Treat nil as "use the default" (ignore it) to make the option safe.
// WithLagAwareHTTPClient sets a custom HTTP client.
func WithLagAwareHTTPClient(client HTTPClient) LagAwareHealthCheckOption {
return func(h *LagAwareHealthCheck) { h.httpClient = client }
}
multidb/healthcheck_lag_aware.go:249
- hostPortFromAddr ignores strconv.Atoi errors for the port. That means addresses like "host:http" (valid for net.Dial via service name lookup) are treated as port=0, which can select the wrong BDB when multiple DBs share a host but differ by port. Resolve service names (LookupPort) or fail fast when the port can't be parsed.
if host, portStr, err := net.SplitHostPort(addr); err == nil {
port, _ := strconv.Atoi(portStr)
return host, port, true
}
multidb/healthcheck_lag_aware.go:74
- WithLagAwareTolerance accepts non-positive values, which can produce a negative/zero availability_lag_tolerance_ms query parameter and change the semantics of the lag check. Since the constructor already sets a default, this option should ignore invalid values (or clamp to the default) to keep behavior predictable.
This issue also appears in the following locations of the same file:
- line 76
- line 246
// WithLagAwareTolerance sets the lag tolerance in milliseconds (default: 5000).
func WithLagAwareTolerance(toleranceMS int) LagAwareHealthCheckOption {
return func(h *LagAwareHealthCheck) { h.lagTolerance = toleranceMS }
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 796679cb2c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Probe the matched BDB's local endpoint availability instead of the database-level check (which reports healthy while ANY endpoint is up), treat redis.Nil as a success in the failure detector so miss-heavy workloads cannot trip it, and publish the open -> half-open transition with a CAS so a concurrent Reset cannot be silently overwritten.
Member
Author
|
@codex review |
Member
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
internal/circuitbreaker/circuit_breaker.go:299
- In the half-open -> open transition, RecordFailure can also end up re-opening the circuit after a concurrent Reset cleared lastFailure. Since CheckState only transitions open->half-open when lastFailure != 0, re-setting lastFailure after a successful CAS to StateOpen avoids a permanently-open breaker.
// Any failure in half-open state opens the circuit.
if cb.state.CompareAndSwap(int32(StateHalfOpen), int32(StateOpen)) {
// Notify callbacks before resetting counters so they observe the
// counts that triggered the transition, matching the half-open ->
// closed path in RecordSuccess.
cb.notifyCallbacks(StateHalfOpen, StateOpen)
cb.successes.Store(0)
cb.requests.Store(0)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 960d96221b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two review findings on the shared availability plumbing: - RecordFailure racing Reset could leave the circuit Open with a zeroed lastFailure, wedging it past CheckState's zero-timestamp guard with no open->half-open path. Repair the timestamp after a successful CAS into Open (CAS from 0 so a newer failure's timestamp is kept). - The failure detector matched server replies via the concrete proto.RedisError string, but the reader parses recognized prefixes into typed structs sharing only the RedisError() marker — NOAUTH, NOPERM, EXECABORT and friends were misclassified as transport failures. Match the marker interface instead, and mirror the root classifier's Lua-embedded READONLY substring so EVAL writes against a replica count as availability failures. Also document that WithLagAwareBaseURL must reach the member's own node for the /v1/local/ endpoint-availability probe to be meaningful.
IsAllowed admits closed-state requests without reserving a probe slot, so a caller whose operation outlives a later open -> half-open transition (a WATCH transaction holding its release until the user function returns) would free a slot a real recovery probe is holding. Allow exposes whether the admission reserved, keeping IsAllowed as the convenience wrapper.
Member
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (4)
multidb/healthcheck.go:296
- Same time.After-in-loop issue as above: this allocates an un-stoppable timer per probe delay and can't be canceled early, which can add avoidable churn under frequent health checks. Use a stoppable time.NewTimer and stop/drain it on ctx cancellation.
if i < cfg.Probes-1 && cfg.Delay > 0 {
select {
case <-ctx.Done():
return false
case <-time.After(cfg.Delay):
}
multidb/healthcheck.go:317
- Same time.After-in-loop issue as above: this allocates an un-stoppable timer per probe delay and can't be canceled early, which can add avoidable churn under frequent health checks. Use a stoppable time.NewTimer and stop/drain it on ctx cancellation.
if i < cfg.Probes-1 && cfg.Delay > 0 {
select {
case <-ctx.Done():
return false
case <-time.After(cfg.Delay):
}
multidb/healthcheck.go:259
- Using time.After inside the probe loop allocates an un-stoppable timer on every iteration; if the context is canceled early, those timers still fire later and can accumulate under frequent health checks. Prefer a stoppable time.NewTimer so ctx cancellation can stop/drain the timer immediately (apply the same pattern to the other probe runners too).
This issue also appears in the following locations of the same file:
- line 291
- line 312
if i < cfg.Probes-1 && cfg.Delay > 0 {
select {
case <-ctx.Done():
return false
case <-time.After(cfg.Delay):
}
multidb/healthcheck.go:78
- MultiDBHealthCheck methods return (bool, error) specifically so callers can record why a check failed, but HealthCheckPolicy only returns bool and the probe runners discard the returned error entirely. That makes it hard for MultiDB failover logic/metrics to attribute unhealthy results to a concrete cause (REST status, TLS config error, ping error, etc.). Consider extending HealthCheckPolicy (and runChecks) to propagate the first/most-recent non-nil error from a failing probe, or adding a callback/hook that receives the probe error.
type HealthCheckPolicy interface {
Execute(ctx context.Context, checks []redis.MultiDBHealthCheck, client *redis.Client) bool
ExecuteCluster(ctx context.Context, checks []redis.MultiDBHealthCheck, client *redis.ClusterClient) bool
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40d540fda6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The claim-then-zero bucketFor still lived on this branch: a writer claiming a stale bucket zeroes its counters after the epoch CAS, wiping any increment that raced in between (one lost count flaked the concurrent-record test in CI on this PR). The immutable-epoch bucketState swap that fixes it landed upstack with the client work; bring the detector file down to the branch that owns it — the client rebase merges clean because both sides carry identical content.
Execute and the maintnotifications handoff gate admitted closed-state work via IsAllowed, then settled with RecordSuccess/releaseRequest as if a half-open slot were held. Work admitted while closed that outlives a later open -> half-open transition then freed a slot a real recovery probe was holding, letting concurrent probes exceed the configured bound. Both settle through Allow's reserved flag now: unreserved successes count via RecordExternalSuccess, unreserved init-error releases are skipped.
Member
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
multidb/healthcheck_lag_aware.go:262
- hostPortFromAddr ignores strconv.Atoi errors (port becomes 0), so an invalid address like "redis.example.com:abc" is treated as a valid host with no port and can incorrectly match a database endpoint. Treat non-numeric ports as an invalid address and return ok=false so callers surface an explicit error.
if host, portStr, err := net.SplitHostPort(addr); err == nil {
port, _ := strconv.Atoi(portStr)
if host == "" {
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ec15fbf8f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
hostPortFromAddr swallowed the Atoi error, so a service-name port
("redis.example.com:redis" — a valid dial target) degraded to the
port-0 wildcard and bdbMatchesHost could match the wrong database on
Enterprise setups where several BDB endpoints share one DNS name.
Resolve service names through the local services database; names it
cannot resolve degrade to host-only matching as before.
Member
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (2)
multidb/healthcheck_test.go:202
- The "TLS options are applied" test doesn't actually verify that the RootCAs PEM was parsed successfully.
WithLagAwareRootCAsrecords a config error whenAppendCertsFromPEMfails, but this test would still pass because it only checksRootCAs != nil(which is set even on parse failure). Using the existinggenTestCAPEM(t)helper and assertinghc2.configErr == nilmakes the test meaningful and avoids relying on a hard-coded PEM blob.
// Test RootCAs with PEM data
caPEM := []byte(`-----BEGIN CERTIFICATE-----
MIIBkTCB+wIJAKHBfpegAzYCMA0GCSqGSIb3DQEBCwUAMBExDzANBgNVBAMMBnVu
dXNlZDAeFw0yMzAxMDEwMDAwMDBaFw0yNDAxMDEwMDAwMDBaMBExDzANBgNVBAMM
BnVudXNlZDBcMA0GCSqGSIb3DQEBAQUAA0sAMEgCQQC7o96WoVCH9xgnLRkMz8pN
.github/workflows/doctests.yaml:7
on.pull_request.branchesfilters by the PR base branch, not the source/head branch. Addingfeature/**here likely won't make doctests run for PRs coming from feature branches intomaster(those already match because the base ismaster). If the intent was to include PRs targeting onlymasterandexamples, this entry should be removed.
branches: [master, examples, 'feature/**']
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3920fe3e58
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ForEachShard probes replicas too, so a dead replica failed the whole member and could trigger failover away from a cluster whose masters were all serving. MultiDB rejects the replica-routing options, so member traffic only reaches masters — probe exactly those.
Member
Author
|
@codex review |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (3)
multidb/healthcheck_test.go:26
- The healthy-client probe should use the same Redis port selection convention as the rest of the repo (REDIS_PORT, default 6380) instead of hard-coding 6379. Otherwise this subtest will often skip even when the standard test stack is up, reducing effective coverage of PingHealthCheck.
client := redis.NewClient(&redis.Options{Addr: "localhost:6379"})
defer client.Close()
multidb/healthcheck.go:252
- probeFunc returns (bool, error) and its comment says the error explains why a probe failed, but the probe runners drop that error (
ok, _ := probe(...)). As a result, callers of HealthCheckPolicy only get a boolean and cannot record/propagate the failure reason (which undermines the MultiDBHealthCheck contract in multidb_healthcheck.go that errors are meant to be surfaced for metrics). Consider changing HealthCheckPolicy/runner plumbing to return (ok bool, err error) and have runChecks return the first error encountered (or aggregate), so the failure reason is not lost.
for i := 0; i < cfg.Probes; i++ {
if ok, _ := probe(ctx, hc); !ok {
return false
}
multidb/healthcheck_test.go:21
- This test suite hard-codes "localhost:6379" for the PingHealthCheck integration probe. The main go-redis test stack defaults to a configurable REDIS_PORT (default 6380; see main_test.go), so this can lead to unnecessary skips or failures when the suite is run against the standard docker-compose stack or a custom port. Importing os enables reading REDIS_PORT so the test can target the same Redis instance as the rest of the suite.
This issue also appears on line 25 of the same file.
import (
"context"
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rand"
"crypto/tls"
"crypto/x509"
"crypto/x509/pkix"
"encoding/pem"
"io"
"math/big"
"net/http"
"net/url"
"strings"
"testing"
"time"
"github.com/redis/go-redis/v9"
)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc3d9ad97
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // CAS, not Store: a concurrent Reset may have just published | ||
| // Closed, and overwriting it with HalfOpen would silently | ||
| // undo the reset. | ||
| if cb.state.CompareAndSwap(int32(StateOpen), int32(StateHalfOpen)) { |
There was a problem hiding this comment.
When an expired Open breaker races with a stale in-flight operation that was admitted before the breaker opened, this CAS can still publish HalfOpen after the inner lastFailure check: RecordFailure updates lastFailure without taking transitionMu, so a fresh failure stored between the load and this CAS sees state Open and will not reopen the breaker. Fresh evidence is that the timestamp writer is not serialized with the locked re-check, so the configured OpenTimeout can be skipped from the latest failure and recovery probes can be admitted immediately; make the transition conditional on the timestamp still being the one that was checked, or serialize open-state failure recording with this transition.
Useful? React with 👍 / 👎.
Comment on lines
+445
to
+446
| if err := json.NewDecoder(resp.Body).Decode(&bdbs); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
When /v1/bdbs returns HTTP 200 with malformed or otherwise undecodable JSON, this branch returns before the bounded drain below, so the default HTTP transport cannot reuse that REST API connection. In a lag-aware health-check loop against a misconfigured proxy or partially failing admin API that keeps producing bad 200 responses, every probe burns a new TCP/TLS handshake; drain the body before returning the decode error, just like the non-2xx and success paths.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.