Description
exponential_backoff/jitter in core/connectors/sdk/src/retry.rs have two
bugs, both reproduced by every call site that composes them by hand:
- Off-by-one doubles the first retry's delay.
exponential_backoff(base, attempt, max_delay) expects a 0-based attempt (0 → base). Most call sites increment their counter before calling it, so the first retry passes 1 and waits base * 2 instead of base, and every subsequent retry is skewed one step ahead too.
- Jitter is applied after the cap, so it can overshoot
max_delay. exponential_backoff clamps to max_delay internally, but jitter(...) (±20%) wraps that already-capped value with no re-clamp: a retry can sleep up to max_delay * 1.2, silently exceeding the configured ceiling.
Affected area / component
Connectors, Rust SDK
| Location |
Off-by-one |
Missing clamp |
SDK retry.rs: HttpRetryMiddleware::handle, check_connectivity_with_retry |
Yes |
Yes |
influxdb_sink / influxdb_source (inherit the SDK bug directly) |
Yes |
Yes |
meilisearch_sink::check_connectivity (3 call sites) |
Yes |
Yes |
core/connectors/sources/iggy_source (passes consecutive_failures directly) |
Yes |
Yes |
s3_sink upload retry loop |
Fixed (attempt - 1) |
Still overshoots |
surrealdb_sink write retry loop |
Fixed (saturating_sub(1)) |
Still overshoots |
doris_sink Stream Load retry |
Fixed |
Fixed |
opensearch_sink::sleep_before_retry |
Fixed (retries - 1) |
Fixed |
SDK source.rs::nack_retry_delay |
Fixed (saturating_sub(1)) |
N/A (never calls jitter) |
clickhouse_sink's own local jittered_backoff helper (4 call sites) |
Yes (separate reimplementation) |
N/A (self-bounding by construction) |
Proposed solution
Fix the interface, not every call site: change exponential_backoff to
accept a 1-based total-attempt count internally (subtract 1 before computing
the exponent), so the call every caller already makes naturally (an
incremented counter) becomes correct.
- No change needed: SDK's
HttpRetryMiddleware/check_connectivity_with_retry (fixes influxdb_sink/influxdb_source too), meilisearch_sink, core/connectors/sources/iggy_source.
- Drop the now-redundant manual
-1: s3_sink, surrealdb_sink, doris_sink, opensearch_sink, SDK source.rs::nack_retry_delay.
- Separate fix, unaffected by the interface change:
clickhouse_sink's local jittered_backoff (own attempts - 1 fix, 4 call sites).
Also add .min(max_delay) after jitter(...) once, in a shared
retry_backoff(base, attempt, max_delay) -> Duration helper, and have every
call site use it instead of composing jitter/exponential_backoff by hand.
This closes the clamp gap in the same pass since those call sites are already
being touched.
Alternatives considered
Fix each call site individually instead of the function.
The current 0-based contract is the one nearly every caller gets wrong (SDK's own middleware, meilisearch_sink, and iggy_source all pass an already-incremented counter). Since the interface itself is being misread by most callers, fixing it at the source removes the problem for good instead of leaving it for the next connector to repeat.
Contribution
Good first issue
Description
exponential_backoff/jitterincore/connectors/sdk/src/retry.rshave twobugs, both reproduced by every call site that composes them by hand:
exponential_backoff(base, attempt, max_delay)expects a 0-basedattempt(0→base). Most call sites increment their counter before calling it, so the first retry passes1and waitsbase * 2instead ofbase, and every subsequent retry is skewed one step ahead too.max_delay.exponential_backoffclamps tomax_delayinternally, butjitter(...)(±20%) wraps that already-capped value with no re-clamp: a retry can sleep up tomax_delay * 1.2, silently exceeding the configured ceiling.Affected area / component
Connectors, Rust SDK
retry.rs:HttpRetryMiddleware::handle,check_connectivity_with_retryinfluxdb_sink/influxdb_source(inherit the SDK bug directly)meilisearch_sink::check_connectivity(3 call sites)core/connectors/sources/iggy_source(passesconsecutive_failuresdirectly)s3_sinkupload retry loopattempt - 1)surrealdb_sinkwrite retry loopsaturating_sub(1))doris_sinkStream Load retryopensearch_sink::sleep_before_retryretries - 1)source.rs::nack_retry_delaysaturating_sub(1))jitter)clickhouse_sink's own localjittered_backoffhelper (4 call sites)Proposed solution
Fix the interface, not every call site: change
exponential_backofftoaccept a 1-based total-attempt count internally (subtract 1 before computing
the exponent), so the call every caller already makes naturally (an
incremented counter) becomes correct.
HttpRetryMiddleware/check_connectivity_with_retry(fixesinfluxdb_sink/influxdb_sourcetoo),meilisearch_sink,core/connectors/sources/iggy_source.-1:s3_sink,surrealdb_sink,doris_sink,opensearch_sink, SDKsource.rs::nack_retry_delay.clickhouse_sink's localjittered_backoff(ownattempts - 1fix, 4 call sites).Also add
.min(max_delay)afterjitter(...)once, in a sharedretry_backoff(base, attempt, max_delay) -> Durationhelper, and have everycall site use it instead of composing
jitter/exponential_backoffby hand.This closes the clamp gap in the same pass since those call sites are already
being touched.
Alternatives considered
Fix each call site individually instead of the function.
The current 0-based contract is the one nearly every caller gets wrong (SDK's own middleware,
meilisearch_sink, andiggy_sourceall pass an already-incremented counter). Since the interface itself is being misread by most callers, fixing it at the source removes the problem for good instead of leaving it for the next connector to repeat.Contribution
Good first issue