fix(table): validate retry duration properties

[fallintoplace]

Summary

Commit retry properties are validated before unsigned values are converted to signed durations or used as retry counts.

• reject durations larger than the maximum safe time.Duration;
• reject reversed minimum/maximum waits;
• normalize explicit zero waits and total timeout to their defaults;
• cap retry counts at a practical math.MaxUint32 limit;
• keep exponential backoff and jitter arithmetic overflow-safe;
• report the offending property in configuration errors.

A full-width PropUInt64 helper preserves values long enough to validate them without truncation.

Testing

• go test . ./table
• boundary coverage for math.MaxUint64, maxRetryDurationMs + 1, reversed waits, zero timeout, and retry-count overflow

[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?