Repository navigation
Add proportional jitter to the reconnect delays - #1255
Merged
Merged
Conversation
The first entry of defaultRetryDelaysInMs is 0 and jitter only applied from the third attempt onwards, so every client disconnected by the same event reconnected at the same instant, then again in lockstep 300ms later. Only from the third attempt did they start to spread. The jitter that was applied was also additive (a flat 0-1000ms), which is disproportionate at both ends: it swamps a 300ms delay and barely moves a 7000ms one. Extract the delay into reconnectDelayInMs and scale each delay by a random factor in [0.5, 1.5). The first retry has no base delay to scale, so it draws from a fixed 0-500ms window; a zero at any later index still passes through as an immediate retry. Port of livekit/client-sdk-js#2124. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xianshijing-lk
requested review from
changt,
cloudwebrtc and
hiroshihorie
as code owners
October 23, 2026 22:23
changt
reviewed
Oct 23, 2026
changt
approved these changes
Oct 23, 2026
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The delay is rounded to whole milliseconds, so a draw near the top of the window rounds up to the bound itself and the exclusive assertion failed roughly one run in a thousand. Assert the inclusive bound, and raise the iteration count so the edge is actually exercised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
500ms is longer than the 300ms that follows it in this schedule, which delays recovery from a brief blip for no added benefit. The window only has to pull a herd of clients apart, so 150ms is enough. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Agreed — 500ms is longer than the 300ms that immediately follows it in this schedule, so it was slowing down the common case (a brief blip) without buying anything. The window only needs to be wide enough to pull a herd of clients apart, not to back off. Dropped it to 150ms in 0e1a0f5. The first retry still fires fast, and 150ms of spread is plenty to decorrelate clients hitting the SFU at once. This is now a deliberate divergence from the JS SDK, which uses 500ms — worth noting in case you'd rather keep strict parity across SDKs. The constant is |
defaultRetryDelaysInMs is a fixed const with no policy hook, so the "explicit immediate retry at a later index" case carried over from the JS SDK cannot happen here. Key the fixed window on the zero base delay alone, which is the actual reason it is needed.
The other SDK's constant can change independently and the comment would go stale. The comparison belongs in the PR discussion, not the source.
hiroshihorie
approved these changes
Oct 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of livekit/client-sdk-js#2124.
The bug
Engine.reconnectcomputed its delay as:Two problems, the same pair the JS SDK had before #2124:
defaultRetryDelaysInMs[0]is0, so every client disconnected by the same event reconnects at the same instant, then again in lockstep 300ms later. The herd only begins to spread on the third attempt — by which point the server has already taken two synchronised waves.Repro
Added
test/core/reconnect_delay_test.dart. Against the current logic (first commit extracts the delay intoreconnectDelayInMswith behaviour unchanged, so the test exercises the real code):100 callers asking for their first — and second — retry delay all got the identical value.
The fix
Scale each delay by a random factor in
[0.5, 1.5). The first retry has a base delay of0with nothing to scale, so it draws from a fixed 0–500ms window instead; a zero at any later index is an explicit request to retry immediately and still passes through as zero.Verification
flutter test— 471/471 pass (the three assertions above fail before the change).flutter analyze lib/src/core/engine.dartclean.Note
reconnectDelayInMsis marked@internal;defaultRetryDelaysInMswas already public and is unchanged.🤖 Generated with Claude Code