fix(core, go): use uint32 for Go BackoffStrategy fields and harden reconnect RetryStrategy against out-of-range values

[xShinnRyuu]

Summary

The Go BackoffStrategy stored its reconnect parameters as int, while the Rust core represents every retry-strategy value as u32 (ConnectionRetryStrategy in glide-core/src/client/types.rs). This change stores the Go fields as uint32 so they map cleanly into the core, and narrows at the public boundary so an out-of-range value can no longer wrap silently. Beyond that Go-side narrowing, it hardens the core RetryStrategy against the three out-of-range hazards listed below, all of which affect every wrapper, not just Go.

Issue link

This Pull Request is linked to issues:

• GO: Backoffstrategy should use uint32 types for the fields
• Core: reconnect jitterPercent > 100 aborts the process (negative jitter lower bound)
• Core: reconnect numOfRetries = 0 causes an infinite reconnect spin (usize underflow)

Closes #3929
Closes #6675
Closes #6676

Features / Behaviour Changes

• Go: a negative or above-2^32-1 value passed to NewBackoffStrategy, or a negative value passed to WithJitterPercent, now fails client creation with a named error instead of silently wrapping. Previously NewBackoffStrategy(-1, ...) produced a retry count of 4294967295.
• Core (all wrappers): a reconnect jitterPercent above 100 no longer aborts the process. A value above 100 made the lower jitter bound negative and Duration::mul_f64 panicked (with panic = "abort" in the FFI release profile). Such a value is now rejected as a configuration error when the client is created, in the core, so every wrapper reports it rather than aborting. jitterPercent of exactly 100 is still valid and means full jitter.
• Core (all wrappers): a reconnect retry count of 0 no longer underflows the last-delay computation in get_infinite_backoff_dur_iterator (previously number_of_retries as usize - 1 wrapped to usize::MAX).
• Core (all wrappers): a reconnect retry count near u32::MAX no longer stalls the shared runtime. The last bounded delay was found by walking ExponentialBackoff with Iterator::nth, one step per retry, called synchronously from the reconnect tasks in reconnecting_connection.rs and cluster_async/mod.rs on the single-threaded Tokio runtime shared by every client in the process. It is now computed arithmetically.

Implementation

• go/config/config.go: the four private BackoffStrategy fields become numOfRetries uint32, factor uint32, exponentBase uint32, jitterPercent *uint32, matching the core. The public signatures NewBackoffStrategy(int, int, int) *BackoffStrategy and WithJitterPercent(int) *BackoffStrategy are unchanged, including their return types, so this is not a breaking API change. Narrowing happens at the public boundary in a toUint32 helper: an out-of-range value is recorded per field and the field stores 0 rather than wrapping. WithJitterPercent validates against 100. toProtobuf() reports every invalid field at once, and baseClientConfiguration.toProtobuf() surfaces it as invalid reconnect strategy: %w, matching the existing circuit-breaker and compression error wrapping. Because the error is tracked per field, calling a setter again with a corrected value clears it, so the reported error always describes the current state.
• glide-core/redis-rs/redis/src/retry_strategies.rs: RetryStrategy::new is fallible and rejects a jitter_percent above a new MAX_JITTER_PERCENT of 100 with an InvalidClientConfig error. standalone_client.rs and client/mod.rs propagate it, so an out-of-range jitter fails client creation instead of aborting on the first reconnect. get_infinite_backoff_dur_iterator derives its tail delay in closed form: ExponentialBackoff::from_millis(base).factor(factor) yields factor * base^(k+1) at index k, so the exponent is the retry count itself, floored at 1 so a retry count of 0 still reports the first delay. That removes both the nth() walk and the underflow, and needs no fallback branch.
• Docs: the accepted range and out-of-range behavior are documented on the Go struct and both public methods, including that a jitterPercent above 100 is rejected and that the core substitutes its own defaults when factor or exponentBase is 0. The jitter bound, and that a value above it is rejected, is also documented on the Node, Python, and Java BackoffStrategy surfaces, and a stale jitter key in the Node createClient examples is corrected to jitterPercent.

Testing

• go/config/config_test.go: cases cover zero values, 2^32 - 1 maxima, per-field negative and above-max inputs (asserting a named error and a stored 0 rather than a wrapped value), jitter above 100 being rejected, jitter of exactly 100 being accepted, a corrected setter clearing its earlier error, every invalid field being reported together, and error propagation through both standalone and cluster ToProtobuf(). The negative-value cases run unconditionally; only the above-2^32-1 cases are gated behind a skip on platforms where int is 32 bits.
• glide-core/redis-rs/redis/src/retry_strategies.rs: unit tests cover a jitter above 100 being rejected with a field-naming InvalidClientConfig error, a jitter of exactly 100 being accepted, the zero-retry case, a retry count of u32::MAX returning promptly rather than spinning, and the infinite tail matching the last bounded delay.
• Commands run: go test ./config/ (pass), cargo test --features tokio-comp --lib in glide-core/redis-rs/redis (316 pass), cargo test --lib in glide-core (161 pass), plus cargo build --tests in both crates and cargo build in ffi.
• End-user verification through the public Go API: building a client configuration with NewBackoffStrategy/WithJitterPercent and calling ToProtobuf() shows a negative retry count fails with invalid reconnect strategy: numOfRetries must be between 0 and 4294967295, got -1, where it previously wrapped to 4294967295, while WithJitterPercent(150) fails with invalid reconnect strategy: jitterPercent must be between 0 and 100, got 150.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one issue.
• Commit message has a detailed description of what changed and why.
• Tests are added or updated.
• CHANGELOG.md and documentation files are updated.
• Linters have been run (make *-lint targets) and Prettier has been run (make prettier-fix).
• Destination branch is correct - main or release
• Create merge commit if merging release branch into main, squash otherwise.
• Make sure to update the documentation in the valkey-glide-docs repository if necessary

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

go/config/config.go (1)

338-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Validation logic and uint32 narrowing are correct.

The bounds check in toUint32 (Line 395) runs before the uint32(value) conversion, so the static-analysis narrowing warning on that line is a false positive here. The design correctly matches the protobuf uint32 contract in glide-core/src/protobuf/connection_request.proto for number_of_retries, factor, exponent_base, and jitter_percent.

One nuance worth a light suggestion: strategy.err only records the first invalid setter call (Line 396-398). If a caller fixes an earlier mistake by calling the same setter again with a valid value (e.g., calling WithJitterPercent twice), the corrected value is stored, but the stale first error still causes toProtobuf to fail. Consider collecting all validation errors with errors.Join instead of keeping only the first one, so the reported error always reflects the final state of the strategy and callers see every invalid field in one report instead of iterating one error at a time.

♻️ Proposed refactor to aggregate validation errors

 type BackoffStrategy struct {
 	numOfRetries uint32
 	factor uint32
 	exponentBase uint32
 	jitterPercent *uint32
-	// The first out-of-range value seen by a setter, if any.
-	err error
+	// Out-of-range values seen by setters, if any.
+	errs []error
 }

 func (strategy *BackoffStrategy) toUint32(name string, value int, maxValue int64) uint32 {
 	if value < 0 || int64(value) > maxValue {
-		if strategy.err == nil {
-			strategy.err = fmt.Errorf("%s must be between 0 and %d, got %d", name, maxValue, value)
-		}
+		strategy.errs = append(strategy.errs, fmt.Errorf("%s must be between 0 and %d, got %d", name, maxValue, value))
 		return 0
 	}
 	return uint32(value)
 }

 func (strategy *BackoffStrategy) toProtobuf() (*protobuf.ConnectionRetryStrategy, error) {
-	if strategy.err != nil {
-		return nil, strategy.err
+	if err := errors.Join(strategy.errs...); err != nil {
+		return nil, err
 	}
 	...
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@go/config/config.go` around lines 338 - 421, Update BackoffStrategy
validation so repeated setters do not leave stale errors after a value is
corrected, while preserving errors for fields that remain invalid. Replace the
single strategy.err approach used by toUint32 with per-field or equivalent
accumulated validation state, aggregate remaining validation errors in
toProtobuf, and ensure all invalid fields are reported together.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@glide-core/redis-rs/redis/src/retry_strategies.rs`:
- Around line 84-94: Update the fallback duration calculation in the retry
strategy around last_attempt and last_duration to exponentiate using
last_attempt + 1, matching ExponentialBackoff’s nth(k) sequence. Preserve the
existing saturating arithmetic and fallback behavior while ensuring the repeated
tail uses the correct duration.

---

Nitpick comments:
In `@go/config/config.go`:
- Around line 338-421: Update BackoffStrategy validation so repeated setters do
not leave stale errors after a value is corrected, while preserving errors for
fields that remain invalid. Replace the single strategy.err approach used by
toUint32 with per-field or equivalent accumulated validation state, aggregate
remaining validation errors in toProtobuf, and ensure all invalid fields are
reported together.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eb38bb94-f0d0-48c8-9ee3-bdb63b4a95ac

📥 Commits

Reviewing files that changed from the base of the PR and between 4bf2677 and 4389e7b.

📒 Files selected for processing (9)

• CHANGELOG.md
• glide-core/redis-rs/redis/src/retry_strategies.rs
• go/config/config.go
• go/config/config_test.go
• java/client/src/main/java/glide/api/models/configuration/BackoffStrategy.java
• node/src/BaseClient.ts
• node/src/GlideClient.ts
• node/src/GlideClusterClient.ts
• python/glide-shared/glide_shared/config.py

[valkey-review-bot]

The Go narrowing itself reads correctly and the deferred-error plumbing matches the existing invalid compression configuration / invalid circuit breaker configuration pattern. Two points on the parts of the change that reach past Go.

The jitter contract now differs between Go (hard error above 100) and the core plus the three other bindings this PR documents (silent clamp to 100) — worth settling on one contract before this lands, since it is a user-visible divergence introduced in a single change. Separately, the retry-count hardening only covers the 0 end of the range: nth() still walks the iterator, so the 2^32-1 end that the new Go test blesses stalls the shared runtime on the reconnect path.

Unrelated to the code: CONTRIBUTING.md:42 requires a DCO sign-off on every commit and Conventional Commits formatting. Five of the six commits here (a5c977f, cf0d56a, ec912b5, 6c49ece, 4389e7b) carry no Signed-off-by trailer and use a no-mistakes(review) / no-mistakes(document) type; only 8b5f9af is signed off.