Skip to content

fix: session migration triggers a full reconnect instead of a resume - #1197

Merged
hiroshihorie merged 4 commits into
mainfrom
sxian/CLT-3322/flutter-session-migration-triggers-a-full-reconnect-insteadOf-resume
Sep 14, 2026
Merged

fix: session migration triggers a full reconnect instead of a resume#1197
hiroshihorie merged 4 commits into
mainfrom
sxian/CLT-3322/flutter-session-migration-triggers-a-full-reconnect-insteadOf-resume

Conversation

@xianshijing-lk

Copy link
Copy Markdown
Contributor

Fixes CLT-3322.

Problem

When the server initiates a node migration, the SDK performs a full reconnect instead of a resume: RoomReconnectingEvent is emitted, every RemoteParticipant is torn down (one ParticipantDisconnectedEvent each), the client re-joins, and RoomConnectedEvent fires again. It should emit RoomResumingEvent and keep the session intact.

Reported by a customer testing Room.sendSimulateScenario(migration: true) on 2.12.0:

Room reconnecting
Participant disconnected: scanner_camera
Participant disconnected: operator_camera
Participant disconnected: additional_camera
...
Room connected: 48263580-7cfd-499d-a45c-02741ba74483

Migrations are routine on Cloud, so every Flutter client sees its remote participants disappear and re-join, subscriptions rebuilt, and per-participant UI state lost.

Root cause

For a migration the server sends LeaveRequest{Action: RESUME, Reason: MIGRATION} (livekit/pkg/rtc/participant.go, MaybeStartMigration) and then closes the signal socket. RESUME means: reconnect with reconnect=1, keep the session.

Engine's leave handler did the right thing — cleared fullReconnectOnNext and called handleReconnect(ClientDisconnectReason.leaveReconnect). But attemptReconnect immediately re-set the flag:

if (_clientConfiguration?.resumeConnection == DISABLED ||
    [ClientDisconnectReason.leaveReconnect,   // <- always true for a leave-driven reconnect
     ClientDisconnectReason.negotiationFailed,
     ClientDisconnectReason.peerConnectionFailed].contains(reason)) {
  fullReconnectOnNext = true;
}

That list predates protocol v13 (added in #439, when a leave with can_reconnect could only mean a full reconnect). The v13 RESUME branch was ported from client-sdk-js in #574 but the escalation was never updated — so the resume branch has been dead code ever since and every RESUME leave ended up in restartConnection().

Neither reference SDK behaves this way:

  • client-sdk-jsRTCEngine.attemptReconnect escalates only for resumeConnection === DISABLED or a never-connected PeerConnection.
  • rust-sdkson_session_event routes Action::Resume straight into a resume cycle.

Changes

lib/src/core/engine.dart:

  1. Drop leaveReconnect from the escalation list in attemptReconnect. The callers that genuinely need a full reconnect — the RECONNECT leave branch and connection_check/checks/checker.dart — already set fullReconnectOnNext = true themselves.
  2. Stop forcing fullReconnectOnNext = false in the RESUME branch. JS and Rust both treat an escalation as sticky, so a resume that already failed at the media level isn't downgraded back into a resume loop.

test/mock/peerconnection_mock.dart: implement setConfiguration (it threw UnimplementedError; the resume path applies the ReconnectResponse ICE servers to both transports).

Tests

New test/core/leave_action_test.dart:

  • RESUME (migration) → RoomResumingEvent, no RoomReconnectingEvent, no ParticipantDisconnectedEvent, remote participants retained, fullReconnectOnNext back to false.
  • RECONNECTRoomReconnectingEvent and participants dropped, as before.

Confirmed the RESUME test fails against the pre-fix code (times out waiting for RoomReconnectedEvent, because the engine re-joins instead of resuming). Full suite (409 tests), flutter analyze, dart format and import_sorter all clean.

Note for app developers

RoomResumingEvent is the Flutter analog of JS's SignalReconnecting; RoomReconnectingEvent means a full reconnect. Both paths end in RoomReconnectedEvent.

Follow-ups (not in this PR)

  • A full-reconnect request arriving while a reconnect attempt is in flight is dropped: attemptReconnect early-returns on _attemptingReconnect, and the successful attempt's _clearPendingReconnect() cancels the queued retry, leaving fullReconnectOnNext stale-true (which also suppresses the next legitimate RoomDisconnectedEvent). JS consumes the flag at attempt start and re-dispatches in finally; that can't be copied verbatim here because Room reads engine.fullReconnectOnNext during the restart's join to skip fast-connect republishing.
  • Flutter emits RoomConnectedEvent again on a full reconnect (driven off EngineJoinResponseEvent); JS only emits Reconnected. Changing that is a public-behavior change.

🤖 Generated with Claude Code

…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>
devin-ai-integration[bot]

This comment was marked as resolved.

`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>
handleReconnect replaces the pending retry timer together with its reason,
and the reason based escalation ran only when that timer fired. A
Leave{RESUME} arriving right after a peer connection failure therefore
swapped the reason to leaveReconnect and the failed connection was resumed
instead of restarted. Now that leaveReconnect no longer escalates on its
own, decide the escalation in handleReconnect so a later request cannot
drop it. The server side resumeConnection switch stays in attemptReconnect
so the latest ClientConfiguration wins.
The RECONNECT test now answers the re-join and waits for
RoomReconnectedEvent instead of tearing down mid restart, and asserts the
signal URL carries no reconnect flag. Add cases for a stale
fullReconnectOnNext, for resumeConnection DISABLED from the server, and for
a Leave{RESUME} racing a pending peer failure retry. The RESUME test also
checks the ReconnectResponse configuration reached both transports.

E2EContainer gains answerJoin() and a clientConfiguration option so tests
can drive a full reconnect and shape the join response.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Devin Review

Comment thread lib/src/core/engine.dart
Comment on lines +1076 to +1080
if ([
ClientDisconnectReason.negotiationFailed,
ClientDisconnectReason.peerConnectionFailed,
].contains(reason)) {
fullReconnectOnNext = true;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Successful resume leaves stale escalation

During an active resume, a peer failure makes handleReconnect set fullReconnectOnNext for a retry that can be canceled. attemptReconnect clears the retry after success, but not the flag. A later disconnect is suppressed, or the next resume becomes a full reconnect.

Learn more

A peer connection can report failure while resumeConnection is still restoring ICE. This block records the required full reconnect immediately and schedules a retry. If the active resume then reaches connected state, attemptReconnect cancels that retry but leaves fullReconnectOnNext true. The room subsequently drops an EngineDisconnectedEvent because the disconnect handler treats the flag as an active restart.

Example: A signal reconnect starts, then the primary peer connection briefly reports failed before its ICE restart reaches connected. The resume succeeds and its queued full reconnect is canceled. The next ordinary signal loss emits no RoomDisconnectedEvent; alternatively, a later migration performs an unnecessary full reconnect.

Recommended fix: Track a full-reconnect request separately from the flag consumed by the active attempt. After an attempt succeeds, either dispatch any escalation recorded during that attempt or clear it explicitly; do not cancel its retry while retaining only fullReconnectOnNext. Add a regression test where peerConnectionFailed arrives after _attemptingReconnect becomes true and the active resume subsequently succeeds.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@hiroshihorie
hiroshihorie merged commit d245718 into main Sep 14, 2026
15 checks passed
@hiroshihorie
hiroshihorie deleted the sxian/CLT-3322/flutter-session-migration-triggers-a-full-reconnect-insteadOf-resume branch September 14, 2026 07:26
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants