chore: bump to proto v1.51.0, expose server resume fault scenarios in sendSimulateScenario - #1202
Merged
Merged
Conversation
hiroshihorie
requested review from
cloudwebrtc and
xianshijing-lk
as code owners
September 14, 2026 13:27
There was a problem hiding this comment.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
Add disconnectSignalOnResume, disconnectSignalOnResumeNoMessages and leaveRequestFullReconnect to the debug API and forward them in the SimulateScenario request. The two disconnect variants drop the signal socket right after arming so the resume the server is waiting for starts immediately, the same post action client-sdk-js performs. subscriberBandwidth was accepted by Room.sendSimulateScenario but never forwarded to the signal client. It is now sent. The example app lists the three new scenarios in its simulate menu.
hiroshihorie
added a commit
that referenced
this pull request
Sep 14, 2026
Fixes CLT-3324. **Stacked on #1197** (base is that branch, not `main`). Independent of #1198 — they touch different code and can merge in either order. ## Problem Three ways a reconnect request could be lost or altered in `Engine`: 1. **Reason override** — `handleReconnect()` clears the pending timer and reschedules with its own reason, so a later caller (the socket close that follows a server `Leave`) overrides an earlier one, and the escalation implied by the first reason is silently dropped. 2. **Dropped request** — `attemptReconnect()` early-returns on `_attemptingReconnect`, so a full-reconnect request arriving mid-attempt is never acted on. 3. **Stale flag** — a successful attempt calls `_clearPendingReconnect()`, cancelling the queued escalation and leaving `fullReconnectOnNext` stuck true, which also suppresses the next legitimate `RoomDisconnectedEvent`. This is the mechanism behind #1197: pre-fix, whether a migration resumed or full reconnected depended on whether the socket-close handler beat the leave-driven `Timer(0)`. In production the close arrives a round-trip later and loses, so the bug reproduced. ## Fix **(1)** The reason → escalation mapping moves from `attemptReconnect` to `handleReconnect`, where the request originates, so it is captured as state instead of being re-derived later from a reason that may have been replaced. The `resumeConnection == DISABLED` check stays in `attemptReconnect` — that's config, not a request. **(2)/(3)** `fullReconnectOnNext` is consumed at the start of an attempt into a local. From there a `true` value unambiguously means a *new* request arrived while the attempt was running, which the `finally` block dispatches. This is client-sdk-js's pattern; rust-sdks does the equivalent with a sticky `full_reconnect |=`. **API note.** Consuming the flag up front means it no longer describes the running attempt, which `Room` relied on to skip fast-connect republishing during a full reconnect's re-join (and to suppress the mid-reconnect disconnect event). Added `Engine.isFullReconnectInProgress` for that question and pointed `Room` at it. `fullReconnectOnNext` keeps its meaning as the *pending request*, so `sendSimulateScenario(fullReconnect: true)` and the connection check are unaffected. Also aligns the failure path with js/rust: a failed full reconnect stays a full reconnect. ## Tests `test/core/reconnect_request_dispatch_test.dart`. The first test ports rust-sdks' `test_resume_escalation_sticks_across_cycles` (`livekit/tests/peer_connection_signaling_test.rs`), which needs a live SFU, two participants and a published sine track and observes the escalation via `LocalTrackRepublished`. The mock transport lets us inject the concurrent request directly and observe it as `RoomReconnectingEvent`, which only the full path emits. - full-reconnect request injected mid-resume → cycle 1 still resumes, cycle 2 re-joins - `peerConnectionFailed` followed by a `signal` request → still re-joins, does not resume - successful resume → neither flag left set Verified both behavioral tests fail against the pre-fix engine (test 1: cycle 2 never happens; test 2: `reconnect=1`, i.e. it resumed). Full suite (412 tests), `flutter analyze`, format and import_sorter all clean. --- ## Update 2026-09-14 (hiroshi) Additive changes on top of the original PR, after taking it over: **Fix (1) already landed.** The reason to escalation move into `handleReconnect` shipped on `main` with #1197, so this branch now only carries the consume and dispatch change plus the two fixes below. Synced with `main` by merge, no history rewrite. **Two more ways a request could vanish** (`d905f517`): - `restartConnection` cleared `fullReconnectOnNext` after joining. The flag was already consumed when the attempt started, so a true value there was a new request, typically a `RECONNECT` leave from the node just joined, and the reset erased it before the `finally` dispatch. It is no longer cleared there. - `resumeConnection` emitted Resumed without checking the signal socket. If the socket dropped while the peer connections were being restored, the attempt reported success on a dead connection and its success path cancelled the retry the drop had scheduled. It now re-checks the socket before emitting Resumed and throws a recoverable `ConnectException`, so the retry path runs another resume. Same check as client-sdk-js and rust-sdks. **Logging** (`467ec5c4`): the `catch` in `attemptReconnect` now logs why an attempt failed. **Tests added** to `test/core/reconnect_request_dispatch_test.dart`: - a peer failure reported through `handleReconnect` mid-resume is dispatched as a full reconnect afterwards - a `RECONNECT` leave arriving mid-restart is not lost (fails without the fix) - a signal drop right behind the `ReconnectResponse` is retried instead of reported as success (fails without the fix) **Verified on Cloud** with the `disconnectSignalOnResume` scenario from #1202, where the server answers the resume and then cuts the socket. This branch, one second to a working connection, one `RoomReconnectedEvent`: ``` 23:44:36 Handle ReconnectResponse 23:44:36 Signal disconnected DisconnectReason.disconnected 23:44:36 resumeConnection: primary is connected: true 23:44:36 attemptReconnect: resume failed: [ConnectException] resumeConnection: signal connection severed during resume 23:44:36 WebSocket reconnecting in 300 ms, retry times 1 23:44:36 Handle ReconnectResponse 23:44:36 emit (public) RoomReconnectedEvent() ``` `main` at `12ec8ef3`, six seconds, two `RoomReconnectedEvent`s, the first on a dead socket: ``` 23:48:15 Handle ReconnectResponse 23:48:15 Signal disconnected DisconnectReason.disconnected 23:48:15 resumeConnection: primary is connected: true 23:48:15 emit (public) RoomReconnectedEvent() 23:48:15 Could not send message, socket not connected (x17, the ICE restart candidates) 23:48:20 onDisconnected reason:peerConnectionClosed 23:48:21 resumeConnection: primary is connected: false 23:48:21 emit (public) RoomReconnectedEvent() ``` The false success on `main` also silently dropped every trickle candidate for the ICE restart, so recovery came from a media failure rather than from the signal layer. Full suite 418, analyzer, format and import order clean. **Retry limit** (`5baf75e9`): the `SignalConnectedEvent` handler reset `_reconnectAttempts` to zero. A resume opens its socket before the peer connections are restored, so any attempt failing after that point, a media timeout or the severed socket check above, started counting from zero again and the retry limit was unreachable. Removed; `_clearPendingReconnect` resets on a completed attempt and `cleanUp` on disconnect, matching client-sdk-js. New test severs three consecutive resumes and checks the scheduled attempts climb 2, 3, 4. Without the fix they read 2, 2, 2. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Hiroshi Horie <548776+hiroshihorie@users.noreply.github.com>
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
1 participant
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.
No description provided.