Repository navigation
Conversation
…region failover When a full reconnect joined a region whose peer connection then failed, that region's handleSignalRestarted kept waiting and resolved on the next region's Restarted, so Reconnected fired twice. The failed region's join also overwrote roomInfo, and the next region cleared the held-back RoomMetadataChanged event, so the change was never reported. Each full reconnect attempt now starts from the room info from before the outage with no held-back events, and only the latest attempt reports the recovery. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
waitForRestarted rejects on the next Restarting, so the handler of a region that failed returns without emitting Reconnected. This replaces the restartAttempt counter in CoreRoom. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
handleSignalRestarted waited for Restarted only after the republish. A failed region whose republish outlasted its failure started the wait after the next Restarting, and the short-circuit on a pcState left over from before the outage could resolve it at once. The wait now starts when SignalRestarted fires, and waitForRestarted always waits for the attempt's Restarted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 2c5ae31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!(await restarted)) { | ||
| // this region failed and the next attempt reports the recovery, or reconnection failed | ||
| // and handleDisconnect is being invoked already | ||
| return; | ||
| } | ||
| this.log.debug(`fully reconnected to server`, { region: joinResponse.serverRegion }); | ||
| this.roomInfoBeforeRestart = undefined; | ||
| this.setAndEmitConnectionState(ConnectionState.Connected); |
There was a problem hiding this comment.
🔴 Late republish falsely completes another reconnect
When republishAllTracks outlasts a successful restart and another restart begins, the older handler still sees restarted as true. It marks the room connected during the new restart and clears its metadata baseline, losing changes reported by later regions.
Learn more
A full reconnect emits Restarted when its peer connection is ready, without waiting for the room's track republishing. The room waits for track republishing before announcing recovery. If another full reconnect begins in that interval, the first attempt's restarted promise is already true, so its eventual continuation clears the saved room state and changes the room back to Connected while the second attempt is in progress. restartConnection emits Restarting again for that second attempt; handleRestarting had kept the original room state for comparison. Clearing it early makes a later failed-region metadata update the baseline for a subsequent attempt.
Example: Region A emits Restarted but camera republishing remains pending. Region B starts a new full restart. The camera republish finishes, and A announces Reconnected during B's restart and discards the saved metadata from before A. If B fails and region C joins with B's metadata, the metadata change can go unreported.
Recommended fix: Associate each handleSignalRestarted run with its restart generation and check that it remains current after republishing and before clearing roomInfoBeforeRestart or announcing recovery. Increment the generation on handleRestarting, and invalidate it on disconnect.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.once(EngineEvent.Restarted, onRestarted); | ||
| this.once(EngineEvent.Disconnected, onDisconnected); | ||
| this.once(EngineEvent.Disconnected, onFailed); | ||
| this.once(EngineEvent.Restarting, onFailed); |
There was a problem hiding this comment.
🟡 Engine closure strands pending restart waits
If the user disconnects after SignalRestarted, waitForRestarted never settles. close emits Closing and removes listeners without emitting any event the wait observes.
Learn more
A restart waiter listens for success (Restarted) or failure (Disconnected or another Restarting). A user-initiated disconnect instead calls close, which emits Closing and removes all engine listeners. Once the close removes these listeners, a pending restart promise cannot resolve or reject; handleSignalRestarted remains suspended at that wait after track republishing finishes.
Example: The engine emits SignalRestarted for a new region. Before its peer connection reconnects, the application calls room.disconnect(). The room disconnects, but its pending handleSignalRestarted never finishes.
Recommended fix: Subscribe to EngineEvent.Closing as a failure in waitForRestarted, and remove that listener in cleanup alongside the other listeners.
Was this helpful? React with 👍 or 👎 to provide feedback.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Generally makes sense to me.
| @@ -1743,13 +1759,13 @@ export class CoreRoom extends (EventEmitter as new () => TypedEmitter<RoomEventC | |||
| this.log.error('error trying to re-publish tracks after reconnection', { error }); | |||
| } | |||
|
|
|||
| try { | |||
| await this.engine.waitForRestarted(); | |||
| this.log.debug(`fully reconnected to server`, { region: joinResponse.serverRegion }); | |||
| } catch { | |||
| // reconnection failed, handleDisconnect is being invoked already, just return here | |||
| if (!(await restarted)) { | |||
| // this region failed and the next attempt reports the recovery, or reconnection failed | |||
| // and handleDisconnect is being invoked already | |||
| return; | |||
There was a problem hiding this comment.
nitpick: is it worth keeping around the fully reconnected to server log line?
| it('reports the recovery once, with the changes the connected region reports', async () => { | ||
| const room = new Room(); | ||
| room.state = ConnectionState.Connected; | ||
| room.engine.emit(EngineEvent.RoomUpdate, join('old').room!); | ||
| vi.spyOn(room.localParticipant, 'republishAllTracks').mockImplementation(async () => {}); | ||
| const reconnected = vi.fn(); | ||
| const metadataChanged = vi.fn(); | ||
| room.on(RoomEvent.Reconnected, reconnected); | ||
| room.on(RoomEvent.RoomMetadataChanged, metadataChanged); |
There was a problem hiding this comment.
nitpick: You might consider using subscribeToEvents (link) here, as it cleans up a lot of this room.on(RoomEvent.Reconnected, fi.fn()); type mocking.
This branch has not been deployed
2 participants
A full reconnect that fails over to a second region reports its recovery twice and can lose a room metadata change. The first region's join succeeds and emits
SignalRestarted, but its peer connection fails. ItshandleSignalRestartedkeeps waiting inwaitForRestarted(), which rejects only onDisconnected. When the second region emitsRestarted, both calls continue, andRoomEvent.Reconnectedfires twice. The first region's join also writes the new metadata intoroomInfoand holds backRoomMetadataChanged. The second region'shandleSignalRestartedclears the held-back events, and its ownhandleRoomUpdatesees no change. The app never gets the event. The same applies toRecordingStatusChanged.This change makes each region attempt start from the room as it was before the outage.
handleRestartingsavesroomInfoonce per outage inroomInfoBeforeRestart, resetsroomInfoto it, and clears the held-back events. The saved room info is cleared after recovery and on disconnect.waitForRestartedalso rejects on the nextRestarting. The failed region'shandleSignalRestartedthen returns without emittingReconnected.On
SignalRestarted, the wait starts before the republish. A failed region's republish can outlastpeerConnectionTimeout(15 s by default). For example, the region answers the join and then stalls on signaling. A wait that starts after such a republish misses the nextRestarting.waitForRestartedalso drops its shortcut forpcState === Connected. The shortcut let a late wait resolve afterRestartedhad already fired. The wait now always starts beforeRestarted, so the shortcut has no use. It is also wrong at that point. The engine setspcStatetoReconnectingonly afterSignalRestarted, so the wait sees the value from before the outage. After a server leave withRECONNECT, that value is stillConnected. The shortcut then resolves the wait before the new peer connection is up.