fix(sse): saturate exponential reconnect backoff to avoid overflow panic - #1231
fix(sse): saturate exponential reconnect backoff to avoid overflow panic#1231ump45nose wants to merge 2 commits into
Conversation
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.
github-actions
Bot
added
T-core
| let multiplier = 2u32.saturating_pow(current_times as u32); | ||
| Some(self.base_duration.saturating_mul(multiplier)) |
There was a problem hiding this comment.
Once the multiplier reaches its limit, the delay can be decades long. The caller passes it directly to tokio::time::sleep, so the stream neither reconnects nor terminates.
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.
|
Thanks for the catch — you're right that a saturating multiplier alone leaves the delay unbounded, and at the saturated value Pushed a follow-up that adds an optional Added a regression test (
|
| Self { | ||
| max_times: None, | ||
| base_duration: Self::DEFAULT_DURATION, | ||
| max_delay: Some(Self::DEFAULT_MAX_DELAY), |
There was a problem hiding this comment.
Because the struct is #[non_exhaustive], downstream callers can only create one with default() and then mutate its fields. That means anyone who sets max_times will inherit the new 30-second limit.
Do you see this shorter window as a bug fix that could go into a patch release, or would leaving max_delay set to None be the safer default?
|
Good question — thanks for thinking through the I'd frame it as: the 30s cap is a safety improvement, not the actual bug fix. The bug in #1198 is the overflow/wrap — that's fully resolved by the saturating multiplier + Given that, I'd suggest making
|
2 participants
Summary
fix(sse): saturate exponential reconnect backoff to avoid overflow panic
Verification
cargo test -p rmcp --features client-side-sse --lib client_side_sse (16 passed); cargo clippy -p rmcp --features client-side-sse --lib (no new warnings); cargo fmt --check (clean)
Related to #1198