fix: validate retry duration properties

[fallintoplace]

What

Retry properties were parsed into unsigned values and later converted to signed integers and time.Duration, allowing extreme metadata values to wrap and potentially panic rand.Int64N during commit retries.

This validates retry milliseconds against the largest safe duration, rejects reversed wait bounds and retry-count overflow, returns the offending property in the configuration error, and hardens backoff arithmetic. A full-width PropUInt64 helper preserves values needed for validation.

Tests

• go test ./...
• Boundary coverage for math.MaxUint64, math.MaxInt64+1, reversed min/max, and the largest safe duration.

[zeroshade]

Thorough hardening of the retry-duration parsing (overflow, min>max, jitter-span) — nice. One small thing to confirm (inline).

[zeroshade]

Minor: minWaitMs and maxWaitMs fall back to their defaults when parsed as 0, but totalTimeoutMs doesn't get the same treatment — an explicit commit.retry.total-timeout-ms=0 stays 0. Worth confirming 0 is intended to mean 'no timeout' here rather than falling back to the default like the wait values do.

[laskoviymishka]

This is a solid hardening pass — the overflow-safe unsigned ceiling math in backoffDuration and the min>max / per-property duration bounds are exactly the right shape, and it closes the rand.Int64N panic path cleanly. Nice work.

Almost there. A couple of things I'd want nailed down before merge.

The one I care most about is the numRetries guard. numRetries >= uint64(^uint(0)) only rejects exactly MaxUint64 on a 64-bit build, so MaxUint64-1 slips through, and then numRetries+1 wraps the attempt count to 0 and we silently run zero attempts — the commit never executes and nothing errors. For a PR whose whole point is closing this class of footgun, I'd want it bounded properly (cap to a practical max like MaxUint32, or at least guard numRetries+1 == 0).

Second, I agree with zeroshade's point on totalTimeoutMs, and I think it's a bit more than a confirm-intent nit. minWaitMs/maxWaitMs fall back to their defaults on 0, but totalTimeoutMs doesn't, so an explicit commit.retry.total-timeout-ms=0 survives validation and lands in context.WithTimeout(ctx, 0), which is already expired. Every commit then fails immediately with deadline-exceeded instead of the real error, no retries, no hint the property caused it. Either normalize 0 to the default like the wait values, or treat 0 as unbounded — but it shouldn't mean "expire instantly."

A few smaller things I left inline:

• the jitterSpan overflow guard looks like dead code — I couldn't find an input that fires it given the earlier bounds
• the line-level nolint:gosec is quietly covering the int64 conversions too, not just rand.Int64N
• the reject-boundary test doesn't actually pin maxRetryDurationMs+1
• one open question on whether PropUInt64 should be a method to match the other Properties helpers

Once the numRetries guard and the totalTimeoutMs case are sorted, happy to take another pass and approve.

[laskoviymishka]

This guard doesn't buy us much and has a surprising edge. On a 64-bit build ^uint(0) is MaxUint64, so >= only rejects that exact value — MaxUint64-1 sails through, then numRetries+1 (the totalAttempts count in the loop) wraps to 0 and we silently run zero attempts, so the commit never executes and no error surfaces.

I'd cap to a sane practical bound instead of the type width — reject numRetries > math.MaxUint32, or at minimum guard numRetries+1 == 0. A retry count anywhere near 2^32 is a misconfiguration regardless of platform width. wdyt?

[laskoviymishka]

I don't think this can ever fire. By the time we get here both minWaitMs and maxWaitMs are validated <= maxRetryDurationMs (~9.2e12) and min <= max is enforced just above, so jitterSpan is at least 1 and at most ~9.2e12 — never 0, never past MaxInt64 (~9.2e18).

The real int64 boundary is the int64(ceiling-minMs+1) conversion in backoffDuration, not here, so this check also points a reader at the wrong place. I'd drop the block; if we want a marker, a one-line comment noting the span is always int64-representable given the maxRetryDurationMs bound is more honest than a guard that can't trigger. wdyt?

[laskoviymishka]

These use MaxUint64 and MaxInt64+1, but the actual guard is value > maxRetryDurationMs, and maxRetryDurationMs is MaxInt64 / 1e6 (~9.2e12) — about six orders of magnitude below MaxInt64+1. So nothing here pins the accept/reject boundary: if the guard were accidentally written as > math.MaxInt64 (dropping the divide), every one of these would still pass.

TestReadRetryConfigAcceptsLargestSafeDuration nails the accept side at exactly maxRetryDurationMs; I'd add the mirror on the reject side — a case at maxRetryDurationMs+1 — so the boundary is actually tested.

[laskoviymishka]

The //nolint:gosec is justified in the comment for rand.Int64N (non-security randomness), but it's line-level, so it's also silently swallowing the G115 warnings on the two uint64→int64 conversions here (int64(minMs) and int64(ceiling-minMs+1)). Those are safe — both operands are bounded by maxRetryDurationMs <= MaxInt64 — but the comment doesn't say so, so a reader can't tell the suppression is intentional for them.

I'd expand the justification to mention the int64 conversions are safe because the operands are <= maxRetryDurationMs. Small thing.

[laskoviymishka]

Design question while this is fresh: the other Properties readers are methods (GetBool, GetInt, GetInt64), but PropUInt/PropUInt64 are package-level free functions. Now that we're exporting a second one we're locking the free-function shape in — can't drop it later without a major bump.

I don't feel strongly, and PropUInt already set the free-function precedent, so there's a real consistency-with-precedent argument. But it's worth a deliberate call: do we want (p Properties) GetUInt64(...) to match the rest of the helpers, or keep the free functions with a one-line note on why? wdyt?