Skip to content

Commit 46db531

Browse files
authored
fix(sse): saturate exponential reconnect backoff to avoid overflow panic (#1231)
* fix(sse): saturate exponential reconnect backoff ExponentialBackoff::retry computed the reconnect multiplier with 2u32.pow(current_times). With max_times unset, current_times can reach the bit width, panicking in debug builds and wrapping to a zero delay in release builds for long-lived SSE clients. Use saturating_pow and Duration::saturating_mul so the delay stays monotonic and panic-free. * fix(sse): cap exponential reconnect backoff at a bounded max delay Saturating the multiplier alone can still yield decades-long sleeps once current_times reaches the bit width, pinning the stream in tokio::time::sleep without reconnecting or terminating. Add an optional max_delay (default 30s) that clamps the computed delay, keeping the backoff monotonic and panic-free while guaranteeing the client retries. * fix(sse): default max_delay to None to preserve unbounded backoff Per maintainer feedback, leave ExponentialBackoff::default() unbounded so the fix stays a pure overflow bug-fix. The saturating multiplier removes the debug panic / release wrap from #1198, while max_delay stays opt-in for callers that want a bounded reconnect delay.
1 parent 744b9f9 commit 46db531

2 files changed

Lines changed: 89 additions & 1 deletion

File tree

crates/rmcp/src/transport/common/client_side_sse.rs

Lines changed: 87 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,11 @@ impl Default for FixedInterval {
205205
pub struct ExponentialBackoff {
206206
pub max_times: Option<usize>,
207207
pub base_duration: Duration,
208+
/// Optional upper bound on a single reconnect delay. `None` (the default) preserves the
209+
/// pre-existing unbounded doubling behavior. Once the multiplier saturates near the bit
210+
/// width that can still produce very long sleeps, so callers that need the client to
211+
/// actually reconnect can set `Some(...)` to clamp the delay.
212+
pub max_delay: Option<Duration>,
208213
}
209214

210215
impl ExponentialBackoff {
@@ -216,6 +221,7 @@ impl Default for ExponentialBackoff {
216221
Self {
217222
max_times: None,
218223
base_duration: Self::DEFAULT_DURATION,
224+
max_delay: None,
219225
}
220226
}
221227
}
@@ -227,7 +233,16 @@ impl SseRetryPolicy for ExponentialBackoff {
227233
{
228234
return None;
229235
}
230-
Some(self.base_duration * (2u32.pow(current_times as u32)))
236+
// `current_times` is unbounded when `max_times` is unset, so the exponent can reach
237+
// the bit width. Saturate the multiplier at `u32::MAX` and use saturating multiplication
238+
// for the base duration so the delay stays monotonic and panic-free instead of an
239+
// overflow panic (debug) or a wrapped-to-zero backoff (release).
240+
let multiplier = 2u32.saturating_pow(current_times as u32);
241+
let delay = self.base_duration.saturating_mul(multiplier);
242+
Some(match self.max_delay {
243+
Some(max_delay) => delay.min(max_delay),
244+
None => delay,
245+
})
231246
}
232247
}
233248

@@ -775,4 +790,75 @@ mod tests {
775790
assert!(stream.next().await.is_none());
776791
assert_eq!(attempts.load(Ordering::Relaxed), 0);
777792
}
793+
794+
#[test]
795+
fn exponential_backoff_saturates_at_high_retry_counts() {
796+
// With `max_times` unset, `current_times` can reach the bit width. The old
797+
// `2u32.pow(current_times)` panicked in debug builds and wrapped in release;
798+
// the saturating implementation must return a monotonic, non-zero delay instead.
799+
let policy = ExponentialBackoff {
800+
max_times: None,
801+
base_duration: Duration::from_millis(1),
802+
max_delay: None,
803+
};
804+
let mut previous = Duration::ZERO;
805+
for current_times in [31usize, 32, 63, 64, 100] {
806+
let delay = policy
807+
.retry(current_times)
808+
.expect("unbounded policy never gives up");
809+
assert!(
810+
!delay.is_zero(),
811+
"delay must stay non-zero at {current_times}"
812+
);
813+
assert!(
814+
delay >= previous,
815+
"delay must stay monotonic at {current_times}"
816+
);
817+
previous = delay;
818+
}
819+
}
820+
821+
#[test]
822+
fn exponential_backoff_caps_delay_at_max_delay() {
823+
// An explicit cap keeps the unbounded doubling policy from producing decades-long
824+
// sleeps once the multiplier saturates. The delay must grow monotonically, stop at
825+
// the configured ceiling, and never exceed it.
826+
let policy = ExponentialBackoff {
827+
max_times: None,
828+
base_duration: Duration::from_secs(1),
829+
max_delay: Some(Duration::from_secs(30)),
830+
};
831+
let mut previous = Duration::ZERO;
832+
for current_times in [0usize, 1, 2, 3, 4, 5, 10, 32, 64, 100] {
833+
let delay = policy
834+
.retry(current_times)
835+
.expect("unbounded policy never gives up");
836+
assert!(
837+
delay >= previous,
838+
"delay must stay monotonic at {current_times}"
839+
);
840+
assert!(
841+
delay <= Duration::from_secs(30),
842+
"delay must respect max_delay at {current_times}"
843+
);
844+
previous = delay;
845+
}
846+
// Beyond the ceiling the delay stays pinned at max_delay.
847+
assert_eq!(
848+
policy.retry(100).expect("never gives up"),
849+
Duration::from_secs(30)
850+
);
851+
}
852+
853+
#[test]
854+
fn exponential_backoff_respects_max_times() {
855+
let policy = ExponentialBackoff {
856+
max_times: Some(3),
857+
base_duration: Duration::from_millis(1),
858+
max_delay: None,
859+
};
860+
assert!(policy.retry(0).is_some());
861+
assert!(policy.retry(2).is_some());
862+
assert!(policy.retry(3).is_none());
863+
}
778864
}

crates/rmcp/src/transport/streamable_http_client.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2293,6 +2293,7 @@ mod tests {
22932293
Arc::new(ExponentialBackoff {
22942294
max_times: Some(1),
22952295
base_duration: Duration::ZERO,
2296+
max_delay: None,
22962297
}),
22972298
);
22982299
let mut stream = std::pin::pin!(stream);
@@ -2397,6 +2398,7 @@ mod tests {
23972398
Arc::new(ExponentialBackoff {
23982399
max_times: Some(1),
23992400
base_duration: Duration::ZERO,
2401+
max_delay: None,
24002402
}),
24012403
);
24022404

0 commit comments

Comments
 (0)