Add circuit breaker configuration support - #483
Conversation
Add ClientCircuitBreakerConfiguration to the client configuration, matching the existing implementation in Java, Python, Node, and Go. The circuit breaker logic lives in the Rust core — this adds the config class, serializes it via FFI, and surfaces CircuitBreakerException. - Add CircuitBreakerConfig class with fluent WithXxx() API - Add CircuitBreakerException mapped from error code 4 - Wire config through FFI structs and Rust create_connection_request() - Simplify existing FFI code (then_some/transpose patterns) - Add unit tests for config validation and builder integration - Add integration tests (normal operation + positive trip test) Closes valkey-io#474 Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
70c6ec7 to
9d874f6
Compare
…with ?? default Remove unnecessary if-blocks and use the same ?? default pattern as all other optional config fields in the ConnectionRequest initializer. Signed-off-by: currantw <taylor.curran@improving.com>
… into 474-circuit-breaker Signed-off-by: currantw <taylor.curran@improving.com>
33be236 to
a4e04a2
Compare
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
There was a problem hiding this comment.
Pull request overview
This PR adds circuit breaker configuration support to the Valkey GLIDE C# client by introducing a public CircuitBreakerConfig API, wiring the configuration through the C#↔Rust FFI boundary, and surfacing a dedicated CircuitBreakerException when the core rejects requests due to an open breaker.
Changes:
- Introduces
CircuitBreakerConfigwith fluentWithXxx()setters and FFI serialization. - Wires circuit breaker config through
ConnectionConfigurationand the FFIConnectionRequest/RustConnectionConfig. - Adds
Errors.CircuitBreakerExceptionplus unit/integration tests covering config and behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Valkey.Glide.UnitTests/CircuitBreakerConfigTests.cs | Adds unit tests for default/unset behavior and validation of the new config API. |
| tests/Valkey.Glide.IntegrationTests/CircuitBreakerTests.cs | Adds integration tests to validate circuit breaker config doesn’t break connectivity and trips/rejects when expected. |
| sources/Valkey.Glide/Internals/FFI.structs.cs | Extends the marshalled connection request to include circuit breaker config fields and struct layout. |
| sources/Valkey.Glide/Errors.cs | Adds CircuitBreakerException and maps a new RequestErrorType to it. |
| sources/Valkey.Glide/ConnectionConfiguration.cs | Adds builder/property plumbing to attach circuit breaker config to client connection configuration. |
| sources/Valkey.Glide/CircuitBreakerConfig.cs | New public config type with validation, fluent API, and ToFfi() conversion. |
| rust/src/ffi.rs | Adds FFI-side circuit breaker struct and converts it into the core client request. |
| CHANGELOG.md | Documents the new circuit breaker configuration feature in the changelog. |
Comments suppressed due to low confidence (2)
sources/Valkey.Glide/CircuitBreakerConfig.cs:74
WithFailureRateThresholdcurrently allows NaN/Infinity because the range checks do not catch those values. Passing NaN through FFI can cause undefined circuit breaker behavior in the core. Reject non-finite values explicitly before the range checks.
ArgumentOutOfRangeException.ThrowIfLessThanOrEqual(threshold, 0.0, nameof(threshold));
ArgumentOutOfRangeException.ThrowIfGreaterThan(threshold, 1.0, nameof(threshold));
FailureRateThreshold = threshold;
sources/Valkey.Glide/CircuitBreakerConfig.cs:100
ToFfi()castsOpenTimeout.TotalMillisecondstouint. For very large timeouts (> ~49.7 days), this overflows and can silently wrap in an unchecked cast. Validate an upper bound when setting the value so the marshalled value stays accurate.
ArgumentOutOfRangeException.ThrowIfLessThanOrEqual(openTimeout, TimeSpan.Zero, nameof(openTimeout));
OpenTimeout = openTimeout;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: currantw <taylor.curran@improving.com>
… into 474-circuit-breaker
…rowIfNegative - Add MaxTimeSpan constant to CircuitBreakerConfig; WithWindowSize and WithOpenTimeout now reject values exceeding uint.MaxValue ms - Replace GuardClauses.ThrowIfNegative with ArgumentOutOfRangeException.ThrowIfLessThan throughout - Remove ThrowIfNegative method from GuardClauses - Update documentation to reflect ArgumentOutOfRangeException - Add unit tests for overflow cases - Fix Copilot-reported invalid cref in ConnectionConfiguration.cs Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
|
As mentioned in comments on the PHP PR I think we should ensure consistency between all the clients, the key pieces being (A) having the CB defaults in the client, and (B) not worrying about config edge case validation (until valkey-io/valkey-glide#5937, although you already have it here so that's fine). |
…alidation - Add public default constants matching glide-core (DefaultWindowSize, DefaultFailureRateThreshold, DefaultMinErrors, DefaultOpenTimeout, DefaultConsecutiveSuccesses) - Make MaxTimeSpan public for consumer reference - Properties are now non-nullable with defaults applied - Change FailureRateThreshold from double to float (matches FFI type) - Remove edge-case validation deferred to glide-core (e.g. threshold > 1.0) - Keep zero/negative guards (FFI sentinels) and MaxTimeSpan upper bound - Remove integration tests (to be rewritten separately) - Update unit tests accordingly Signed-off-by: Taylor Curran <taylor@valkey.io> Signed-off-by: currantw <taylor.curran@improving.com>
… into 474-circuit-breaker Signed-off-by: currantw <taylor.curran@improving.com>
|
Made the following changes for consistency with other clients:
However I didn't retain some edge case validation for these two cases:
|
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Summary
Add
CircuitBreakerConfigto the client configuration, matching the existing implementation in Java, Python, Node, and Go (valkey-io/valkey-glide#6050). The circuit breaker logic lives in the Rust core — this task adds the config class, serializes it via FFI, and surfaces theCircuitBreakerExceptiontype to callers.Issue Link
Closes #474.
Features and Behaviour Changes
CircuitBreakerConfigclass with fluentWithXxx()API for configuring the client-wide circuit breakerErrors.CircuitBreakerExceptionthrown when the breaker is open and requests are rejectedConnectionConfigurationbuilder gainsWithCircuitBreaker(CircuitBreakerConfig)methodImplementation
CircuitBreakerConfig.cs: Sealed class with nullable properties, fluent setters with validation, andToFfi()conversionErrors.cs: AddedCircuitBreakerOpen = 4toRequestErrorTypeenum andCircuitBreakerExceptionclassFFI.structs.cs: AddedCircuitBreakerConfigreadonly struct (primary constructor pattern) and wired throughConnectionRequestConnectionConfiguration.cs: Added field toConnectionConfigrecord and builder methodrust/src/ffi.rs: AddedCircuitBreakerConfigFFI struct and conversion increate_connection_request(). Also simplified existing code withthen_some/then/transposepatterns.Limitations
Testing
CircuitBreakerConfigTests.cs): defaults, validation, fluent chaining, builder integrationCircuitBreakerTests.cs):CircuitBreakerExceptionRelated Issues
Checklist
CHANGELOG.md,README.md,DEVELOPER.md, and other documentation files are updated.mainor releasemain, squash otherwise.