Skip to content

Commit

Permalink
Prevents the exponential backoff from overflowing after 64 attempts. (#…
Browse files Browse the repository at this point in the history
…4501)

In release mode, 2^64 is 0, so after 64 attempts, the retry delay was 0.
(In debug mode, rust panics)

Closes #4499
  • Loading branch information
fulmicoton authored Feb 2, 2024
1 parent f38bdfd commit 8342c52
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 6 deletions.
1 change: 1 addition & 0 deletions quickwit/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions quickwit/quickwit-common/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,4 @@ named_tasks = ["tokio/tracing"]
serde_json = { workspace = true }
tempfile = { workspace = true }
quickwit-macros = { workspace = true }
proptest = { workspace = true }
39 changes: 33 additions & 6 deletions quickwit/quickwit-common/src/retry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,13 @@ impl RetryParams {
/// Panics if `num_attempts` is zero.
pub fn compute_delay(&self, num_attempts: usize) -> Duration {
assert!(num_attempts > 0, "num_attempts should be greater than zero");

let delay_ms = self.base_delay.as_millis() as u64 * 2u64.pow(num_attempts as u32 - 1);
let ceil_delay_ms = delay_ms.min(self.max_delay.as_millis() as u64);
let half_delay_ms = ceil_delay_ms / 2;
let jitter_range = 0..half_delay_ms + 1;
let jittered_delay_ms = half_delay_ms + rand::thread_rng().gen_range(jitter_range);
let num_attempts = num_attempts.min(32);
let delay_ms = (self.base_delay.as_millis() as u64)
.saturating_mul(2u64.saturating_pow(num_attempts as u32 - 1));
let capped_delay_ms = delay_ms.min(self.max_delay.as_millis() as u64);
let half_delay_ms = (capped_delay_ms + 1) / 2;
let jitter_range = half_delay_ms..capped_delay_ms + 1;
let jittered_delay_ms = rand::thread_rng().gen_range(jitter_range);
Duration::from_millis(jittered_delay_ms)
}

Expand Down Expand Up @@ -258,4 +259,30 @@ mod tests {
.collect();
assert_eq!(simulate_retries(retry_sequence).await, Ok(()));
}

fn test_retry_delay_does_not_overflow_aux(retry_params: RetryParams) {
for i in 1..100 {
let delay = retry_params.compute_delay(i);
assert!(delay <= retry_params.max_delay);
if retry_params.base_delay <= retry_params.max_delay {
assert!(delay * 2 >= retry_params.base_delay);
}
}
}

proptest::proptest! {
#[test]
fn test_retry_delay_does_not_overflow(
max_attempts in 1..1_000usize,
base_delay in 0..1_000u64,
max_delay in 0..60_000u64,
) {
let retry_params = RetryParams {
max_attempts,
base_delay: Duration::from_millis(base_delay),
max_delay: Duration::from_millis(max_delay),
};
test_retry_delay_does_not_overflow_aux(retry_params);
}
}
}

0 comments on commit 8342c52

Please sign in to comment.