Skip to content

fix: don't drop reconnect requests or lose their escalation - #1199

Open
xianshijing-lk wants to merge 4 commits into
mainfrom
sxian/CLT-3324/reconnect-requests-can-be-dropped-or-have-their-reason-silently
Open

fix: don't drop reconnect requests or lose their escalation#1199
xianshijing-lk wants to merge 4 commits into
mainfrom
sxian/CLT-3324/reconnect-requests-can-be-dropped-or-have-their-reason-silently

Conversation

@xianshijing-lk

Copy link
Copy Markdown
Contributor

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 overridehandleReconnect() 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 requestattemptReconnect() 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.

🤖 Generated with Claude Code

xianshijing-lk and others added 3 commits September 10, 2026 13:46
…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>
Three ways a reconnect request could be lost or altered:

1. `handleReconnect()` clears the pending timer and reschedules with its
   own reason, so a later caller (the socket close following a server
   Leave) overrode an earlier one and the escalation implied by the first
   reason was silently dropped. The reason -> escalation mapping now
   happens in `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.

2. `attemptReconnect()` early-returns while an attempt is in flight, so a
   full-reconnect request arriving mid-attempt was never acted on. The
   flag is now consumed at the start of an attempt and a request that
   arrives during it is dispatched from the finally block, as
   client-sdk-js does.

3. A successful attempt calls `_clearPendingReconnect()`, cancelling the
   queued escalation and leaving `fullReconnectOnNext` stuck true, which
   also suppressed the next legitimate `RoomDisconnectedEvent`. Fixed by
   the same consume-and-redispatch.

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. Added `Engine.isFullReconnectInProgress` for
that question and pointed `Room` at it.

Also aligns the failure path with js/rust: a failed full reconnect stays
a full reconnect.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@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.

Note

This report is out of date. Scroll down for Devin Review's latest report on this PR.

Devin Review found 1 potential issue.

Devin Review

Comment thread lib/src/core/engine.dart
Comment on lines +1230 to +1232
if (succeeded && fullReconnectOnNext && !_isClosed) {
logger.fine('attemptReconnect: full reconnect requested mid-attempt, dispatching');
unawaited(handleReconnect(ClientDisconnectReason.reconnectRetry));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Mid-restart escalation is still dropped

During another full reconnect, fullReconnectOnNext can become false before this dispatch check. restartConnection clears it after joining, so the requested follow-up never runs.

Learn more

A reconnect request received during an attempt sets fullReconnectOnNext and schedules a timer. That timer can return at the _attemptingReconnect guard, leaving the flag as the only record of the request. A full reconnect then reaches restartConnection, which clears that shared flag before attemptReconnect checks it in finally. The follow-up request is therefore indistinguishable from no request and is dropped.

Example: A full reconnect is joining a replacement node when that node sends Leave{RECONNECT}. The handler sets fullReconnectOnNext = true, but the current join later clears it. The SDK reports the current attempt as successful and never performs the newly requested reconnect.

Recommended fix: Stop clearing pending reconnect state inside restartConnection. Let attemptReconnect consume only the request that selected the current attempt, while preserving requests recorded after that consumption point.

Devin Review

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
# Conflicts:
#	lib/src/core/engine.dart
#	test/core/leave_action_test.dart

@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 +1235 to +1237
if (succeeded && fullReconnectOnNext && !_isClosed) {
logger.fine('attemptReconnect: full reconnect requested mid-attempt, dispatching');
unawaited(handleReconnect(ClientDisconnectReason.reconnectRetry));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Resume reconnect requests still disappear

When a resume request arrives during a successful attempt, fullReconnectOnNext remains false and blocks redispatch. _clearPendingReconnect cancels that request's timer, so the new disconnection gets no reconnect attempt.

Learn more

A reconnect request can arrive after the running attempt has passed the operation that prompted it but before that attempt finishes. handleReconnect schedules the new request regardless of its mode. The successful attempt then calls _clearPendingReconnect, cancelling that timer. This condition only preserves requests represented by fullReconnectOnNext, so resume requests still disappear.

Example: A resume reconnects signaling and receives SignalReconnectedEvent. Before its peer-connection work finishes, signaling disconnects again and schedules a resume. The first attempt succeeds, cancels the second timer, and emits success although signaling is now disconnected.

Recommended fix: Track whether any reconnect request arrived during the running attempt separately from its full-reconnect escalation. On success, dispatch the pending request with its captured reason and reconnectReason; preserve full escalation independently.

Devin Review

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

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