fix: reconcile the participant roster after a signal resume - #1198
Open
xianshijing-lk wants to merge 5 commits into
Open
Conversation
…ting
For a node migration the server sends `LeaveRequest{Action: RESUME,
Reason: MIGRATION}`, which asks the client to reconnect with `reconnect=1`
and keep its session. The engine's leave handler did exactly that, but
`attemptReconnect` then unconditionally escalated any `leaveReconnect`
into a full reconnect:
if (... || [ClientDisconnectReason.leaveReconnect, ...].contains(reason)) {
fullReconnectOnNext = true;
}
That list predates protocol v13 (#439), when a leave with `can_reconnect`
could only mean a full reconnect. The v13 RESUME branch ported in #574
never updated it, so the resume branch has been dead code since: every
RESUME leave ran `restartConnection()`, emitting `RoomReconnectingEvent`,
dropping every `RemoteParticipant` and re-joining.
Drop `leaveReconnect` from the escalation list — the callers that do need
a full reconnect (the RECONNECT leave branch, the connection check) set
`fullReconnectOnNext` themselves. Also stop forcing the flag to false in
the RESUME branch: client-sdk-js and rust-sdks both treat an escalation as
sticky, so a resume that already failed at the media level is not
downgraded back into a resume loop.
Adds `test/core/leave_action_test.dart` covering both leave actions, and
implements `setConfiguration` on the mock peer connection (the resume path
applies the `ReconnectResponse` ICE servers).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`reconnect=1` is the query parameter the server actually keys off to distinguish a resume from a re-join, and it was the only part of the resume contract the test wasn't checking. Also documents why the socket close that follows the Leave is not simulated: a bare socket drop reconnects with reason `signal`, which resumes on its own, so delivering the close before the leave-driven attempt runs makes the test pass even when the leave action is ignored. In production the close arrives a round-trip later and never wins that race, which is why the reported bug reproduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A resume, unlike a full reconnect, never rebuilds the roster from a JoinResponse, and the DISCONNECTED update for anyone who left during the outage went to a socket we no longer had. Those participants stayed in `room.remoteParticipants` forever. The server answers a resume with the ReconnectResponse followed immediately by a full roster snapshot on the same socket, so the snapshot is authoritative: any participant we still hold that is absent from it left while we were away and its disconnect is synthesized. Mirrors `reconcile_absent_participants` in rust-sdks. The reconciliation is armed off `SignalReconnectResponseEvent` rather than `SignalReconnectedEvent` — the latter is emitted only after the engine's async ReconnectResponse handling (setConfiguration on both transports, reliable-message replay) and can lose the race against the update that follows it on the wire. Arming off the raw signal message keeps the two ordered. If no snapshot ever arrives the reconciliation simply never fires, so a missing snapshot can never be read as "everyone left". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xianshijing-lk
requested review from
cloudwebrtc and
hiroshihorie
as code owners
September 10, 2026 21:33
The arming had no expiry, so if the server's post-resume snapshot never arrived it sat there indefinitely and the next *ordinary* participant update — which lists only what changed — was mistaken for a full roster and evicted everyone else. That is the one failure direction this feature must never have. Two guards: - The arming now expires after 5s. The snapshot follows the ReconnectResponse on the same socket, so the window only has to cover scheduling, never a real wait; if it lapses the reconciliation simply never runs. - An update only counts as the snapshot if it carries the local participant. The server includes it so metadata changes propagate, which is exactly what distinguishes a full roster from a partial update. This also covers the RoomMoved path, which reuses the same handler with `otherParticipants` (no local entry). Both failure modes are now "no reconciliation", never "evict a live participant". Tests cover a partial update arriving while armed, an update after the window lapses, and escalation to a full reconnect while armed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+922
to
+926
| _resumeRosterSnapshotTimeout = Timer(_resumeRosterSnapshotWindow, () { | ||
| if (_resumeRosterSnapshot != null) { | ||
| logger.fine('resume roster snapshot never arrived, skipping reconciliation'); | ||
| } | ||
| _disarmResumeRosterSnapshot(); |
There was a problem hiding this comment.
🟡 Slow snapshots skip roster reconciliation
When processing an arrived roster takes five seconds, _resumeRosterSnapshotTimeout disarms it before reconciliation. Participant and track updates run sequentially. Participants absent during the outage remain in the room indefinitely.
Learn more
The timer measures time until reconciliation finishes, not time until the snapshot arrives. The roster handler captures the armed set, then awaits each participant update before checking that the set remains armed. Existing participants can perform asynchronous track reconciliation in updateFromInfo. If those operations exceed five seconds, this callback clears the set, so the final identity check rejects the already-arrived snapshot.
Example: A 200-participant snapshot arrives immediately, but sequential track updates take 5.2 seconds. The timer clears _resumeRosterSnapshot at five seconds. The absent participant leaver is never reconciled and remains in remoteParticipants.
Recommended fix: Detect a qualifying batch synchronously when _onParticipantUpdateEvent starts and cancel its arrival timeout immediately. Keep the captured set alive until processing and reconciliation finish, while preserving the identity guard for a later resume.
Was this helpful? React with 👍 or 👎 to provide feedback.
Base automatically changed from
sxian/CLT-3322/flutter-session-migration-triggers-a-full-reconnect-insteadOf-resume
to
main
September 14, 2026 07:26
Comment on lines
+909
to
+911
| if (rosterSnapshot != null && sawLocalParticipant && identical(rosterSnapshot, _resumeRosterSnapshot)) { | ||
| _disarmResumeRosterSnapshot(); | ||
| hasChanged = await _reconcileAbsentParticipants(rosterSnapshot) || hasChanged; |
There was a problem hiding this comment.
🔴 Concurrent local updates evict live participants
While a roster snapshot is processing, a concurrent local-participant update can reconcile its incomplete shared set. _signalListener dispatches asynchronous participant callbacks concurrently, so the later callback can disarm the snapshot first. Present participants receive false disconnects and can be recreated as new arrivals.
Learn more
Participant updates use asynchronous handlers, but the room's signal listener is not synchronized. A snapshot handler can therefore pause while updating one participant, allowing a later update to enter the same method with the same _resumeRosterSnapshot. If that later update contains the local participant, it passes the snapshot test and disarms reconciliation. It then compares the current roster against a set that the original snapshot has only partially populated. The original handler can subsequently recreate participants that the premature reconciliation removed.
Example: The authoritative snapshot contains local Alice, Bob, and Carol. Processing pauses after adding Bob. A queued metadata update for Alice then sees the shared set {Bob}, removes Carol, and disarms reconciliation. The original snapshot later processes Carol as a new participant, producing false disconnect and reconnect events.
Recommended fix: Serialize SignalParticipantUpdateEvent handling, preferably by creating the room signal listener with synchronized: true as required for mutable connection event flow. Alternatively, introduce a dedicated queue or lock around _onParticipantUpdateEvent and keep each snapshot's accumulation private.
Was this helpful? React with 👍 or 👎 to provide feedback.
# Conflicts: # lib/src/core/engine.dart # test/core/leave_action_test.dart
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
2 participants
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.
Fixes CLT-3323. Stacked on #1197 (base is that branch, not
main) — it carries the mocksetConfigurationfix the resume path needs. GitHub will retarget this tomainonce #1197 merges.Problem
After a signal resume (node migration, transient signal drop), participants who left while the link was down are never removed. Their
DISCONNECTEDupdate went to a socket we no longer had, and a resume — unlike a full reconnect — never rebuilds the roster from aJoinResponse. They stay inroom.remoteParticipantsindefinitely.This became reachable once #1197 stopped migrations from full reconnecting. The full reconnect used to unwind the whole roster and rebuild it, which masked the gap; now that migrations correctly keep participants, stale entries are the failure mode.
client-sdk-js has the same gap. rust-sdks fixed it (
reconcile_absent_participants).Fix
The server answers a resume with the
ReconnectResponsefollowed immediately by a full roster snapshot on the same socket (livekit/pkg/rtc/room.go—HandleReconnectAndSendResponse, thenSendParticipantUpdate). That snapshot is authoritative: any participant we still hold that is absent from it left while we were away, so its disconnect is synthesized through the existing_handleParticipantDisconnectpath.The whole design question here is: what counts as the snapshot? Getting that wrong in the permissive direction evicts live participants, so the arming is fenced three ways:
SignalReconnectResponseEvent, notSignalReconnectedEvent. The latter is emitted only after the engine's async ReconnectResponse handling (setConfigurationon both transports, reliable-message replay) and can lose the race against the roster update that follows it on the wire — I hit exactly that while writing the tests.RoomMovedpath, which reuses the same handler withotherParticipants(no local entry).ReconnectResponseon the same socket, so the window only has to cover scheduling, never a real wait.Net: every failure mode is "no reconciliation, ghost survives", never "evict a live participant". Disarmed on full restart (which rebuilds the roster itself) and in
_cleanUp, which also covers dispose.This differs from rust-sdks, which accumulates a union of updates and reconciles after a 1 s
PC_RECONNECT_SETTLE_DELAY. Keying off the snapshot is deterministic and adds no delay toRoomReconnectedEvent.Tests
test/core/resume_roster_reconcile_test.dart, ported from rust-sdks'test_resume_synthesizes_disconnect_for_participant_that_left. That test needs a live SFU plus adrop_disconnected_updatesfault-injection switch; the mock transport gives the same setup for free — we simply never deliver the leaver's disconnect. The observer/leaver/witness shape is kept, so the witness proves reconciliation only removes participants that actually left.ParticipantDisconnectedEvent, witness retainedFull suite (415 tests),
flutter analyze, format and import_sorter all clean.🤖 Generated with Claude Code