fix: stop putting the subscriber into restartingIce on a reconnect - #2054
fix: stop putting the subscriber into restartingIce on a reconnect#2054xianshijing-lk wants to merge 2 commits into
Conversation
`triggerIceRestart` set `subscriber.restartingIce = true` on every resume, but only `setRemoteDescription` cleared it -- and the server re-offers the subscriber only when the resume actually moved the participant to a different node. After the far more common signal-only resume no offer arrives, so the flag stayed set for the lifetime of the transport and `addIceCandidate` queued every subsequent remote candidate instead of applying it, leaving the subscriber unable to adopt any new network path the server proposed. Media could then stall until some unrelated negotiation happened to flush the queue. The publisher never had this problem because it sets the same flag alongside an offer it will certainly receive an answer to. The subscriber's is speculative, so it needs an explicit close. Split the speculative flag out of `triggerIceRestart` into `beginSubscriberIceRestart`, so it sits next to the matching `finishSubscriberIceRestart` in the resume and the pairing is visible at the call site. `finishRestartingIce` clears the flag and applies whatever queued behind it; where the server did re-offer, the description has already cleared it and the call is a no-op. The resume wraps both the ICE restart and the reconnect wait in try/finally so every exit closes the window, including a publisher offer that throws before we ever wait. Candidates queued because no remote description exists yet stay queued -- there is still nothing to apply them against. This also drops the flag from the `updateConfiguration(config, iceRestart)` path, which called `triggerIceRestart` fire-and-forget with no close and so had no way to bound the window. Nothing currently passes `iceRestart`, so this removes a trap rather than changing behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: efdbd39 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| } finally { | ||
| this.pcManager.finishSubscriberIceRestart(); | ||
| } |
There was a problem hiding this comment.
🟡 Reconnect cleanup can crash with a type error, hiding the real reconnect failure
The cleanup step that closes the candidate-buffering window assumes the connection manager still exists (this.pcManager.finishSubscriberIceRestart() at src/room/RTCEngine.ts:1482), so if the room is torn down while the reconnect is still waiting, the cleanup itself throws and replaces the real error.
Impact: A disconnect during a reconnect attempt can surface a confusing internal error instead of the actual reconnection failure.
Manager reference can be cleared during the awaited reconnect wait
resumeConnection null-checks this.pcManager at src/room/RTCEngine.ts:1428, but the reference is cleared by cleanup (this.pcManager = undefined at src/room/RTCEngine.ts:461) which can run while waitForPCReconnected() is awaited. The surrounding code acknowledges this: waitForPCReconnected re-checks if (!this.pcManager) at src/room/RTCEngine.ts:1513. Because the new finally block dereferences the field unconditionally, a TypeError thrown there supersedes whatever error the try body was propagating. Using optional chaining avoids it.
| } finally { | |
| this.pcManager.finishSubscriberIceRestart(); | |
| } | |
| } finally { | |
| this.pcManager?.finishSubscriberIceRestart(); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
size-limit report 📦
|
Supersedes the bounding approach in this PR's first commit. Tracing the server shows the flag guards a sequence it cannot produce, so removal is both simpler and safer than managing the window. On a same-node resume the server ICE-restarts the subscriber via `clearLocalDescriptionSent`, which buffers its local candidates until after the offer has been sent. On a reconnect that lands on another node the participant starts in `MigrateStateInit`, where subscriber candidates are dropped outright, and that state is only left immediately before the offer is created -- at which point ICE gathering has not started. Either way the offer precedes the candidates it belongs to, so queueing them protects nothing. Against that, setting the flag cost two defects. It outlived the reconnect whenever the server did not re-offer -- the common signal-only resume -- leaving the subscriber queueing every later candidate for the rest of the session. And because it was set after the signalling link had already been reopened, an offer that arrived first would leave the window opening *behind* it, withholding the new generation's candidates during the very wait that decides whether the reconnect succeeded. The publisher keeps its own `restartingIce`: it is set alongside an offer that will certainly be answered, so `setRemoteDescription` always clears it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
xianshijing-lk
changed the title
| */ | ||
| async triggerIceRestart() { | ||
| this.iceLog.warn('triggering ICE restart'); | ||
| if (this.subscriber) { |
There was a problem hiding this comment.
What this fixes: after any reconnect, the subscriber silently stops applying
remote ICE candidates for the rest of the session.
triggerIceRestart() set subscriber.restartingIce = true. That flag tells
addIceCandidate to park candidates in pendingCandidates instead of applying
them, and the only code that clears it is setRemoteDescription — which runs on
the subscriber only when the server re-offers it, i.e. only when the reconnect
moved the participant to a different node. After an ordinary signal-only resume
no offer arrives, so the flag stays set for the transport's lifetime and every
later candidate is parked. The subscriber can no longer adopt a new network path
the server proposes (route change, NAT rebind, switch to relay), so media can
stall until some unrelated negotiation happens to flush the queue.
The fix: stop setting it. The queueing existed to stop candidates for a new
ICE generation being applied against the old remote description, but the server
never sends them in that order — a same-node resume buffers its candidates until
after the offer is sent, and a reconnect landing on another node drops subscriber
candidates until immediately before the offer is created. So the flag had no
upside and one permanent downside.
After this change nothing can set subscriber.restartingIce at all: the only
= true is in createAndSendOffer, reachable only for the publisher. The
publisher's own use is sound and untouched — it's set as an offer is sent, and
the answer clears it.
The line worth making sure a reviewer sees is the asymmetry: the publisher sets the flag alongside an offer that will certainly be answered, so it always clears; the subscriber's was set on the expectation of an offer that usually never comes. That's the whole bug in one sentence.
cnderrauber
left a comment
cnderrauber
left a comment
There was a problem hiding this comment.
but the only thing that clears it is PCTransport.setRemoteDescription() — and the server re-offers the subscriber only when the reconnect actually moved the participant to a different node.
Sfu will send offer in the signal-only resume case, does it not arrive in the test?
The flag exists to stop candidates for a new ICE generation being applied against the old remote description. The server does not send them in that order
Agree with the conclusion that sfu will not send candidate before offer since @boks1971 has made an event queue to make sure all these happen in order.
Looks good to me!
2 participants
Problem
PCTransportManager.triggerIceRestart()put the subscriber intorestartingIceon every reconnect:but the only thing that clears it is
PCTransport.setRemoteDescription()— and the server re-offers the subscriber only when the reconnect actually moved the participant to a different node. On the far more common signal-only resume (WS blip, proxy timeout) no offer arrives, so nothing ever clears it, andaddIceCandidatetakes the queueing branch for the rest of the session:The subscriber therefore stops adopting any new network path the server proposes, and media can stall until some unrelated negotiation happens to flush the queue.
There was a second problem with the same flag. It was set after
client.reconnect()had already reopened the signalling link, so if the server's offer arrived first the window opened behind it — withholding the new generation's candidates during the very wait that decides whether the reconnect succeeded, turning a safeguard into a stall.Fix: don't set it
The flag exists to stop candidates for a new ICE generation being applied against the old remote description. The server does not send them in that order, on either path:
ResumeParticipant→ICERestart→createAndSendOffer{ICERestart}→clearLocalDescriptionSent()setscacheLocalCandidates, and candidates are only flushed bylocalDescriptionSent()afterOnOffer.MigrateStateInit, whereonICECandidatedrops subscriber candidates outright. That state is left inonSyncState, immediately beforeNegotiate(false)creates and sends the offer — at which pointSetLocalDescriptionhas not run, so gathering has not started and no candidate exists to send.Either way the offer precedes the candidates it belongs to, so queueing them protects nothing while causing both defects above.
The publisher keeps its own
restartingIce: it is set alongside an offer that will certainly be answered, sosetRemoteDescriptionalways clears it. That one is sound and untouched.Testing
pnpm test— 716 passed.One test, in
PCTransportManager.test.ts:does not stop the subscriber applying remote candidates. It assertstriggerIceRestart()leavessubscriber.restartingIcefalse, which is exactly the reintroduction this change guards against — verified by re-adding the assignment, which fails it.Since the change is a removal, there is no new behaviour to cover beyond that guard, and
PCTransport.test.tsis byte-identical tomainagain.History of this PR
The first commit bounded the window (added a
finishRestartingIceand paired it across every exit). That fixed the leak but kept the ordering hazard and still withheld candidates on every signal-only resume. Reading the server settled that the flag has no upside at all, so the second commit removes it instead. The net diff againstmainis now +47/−4 insrc/.Context
Found while fixing the equivalent bug in the Rust SDK, which had inherited this pattern from here. Both SDKs now leave the subscriber alone across a reconnect.