apache / apache/iggy

Retry backoff has an off-by-one delay and an unclamped post-jitter overshoot

Open
#4,084 4 comments 0 reactions 1 assignee Claimed by @Youmanvi View on GitHub
connectors good first issue rust
Dominant language
Rust
Stars
4.9k
Forks
432
Avg merge
2d 10h
Merged PRs (30d)
173

Description

### 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:

1. **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.
2. **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

- [ ] I'm willing to submit a pull request to implement this feature

### Good first issue

- [x] I think this could be a good first issue for a new contributor

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.