[MM-69687] Fix orphaned session when peer fails during WS reconnect (v1) - #1253
[MM-69687] Fix orphaned session when peer fails during WS reconnect (v1)#1253bgardner8008 wants to merge 3 commits into
Conversation
…reconnect
In v1 (RTCD), if an RTC peer connection failed while the plugin WebSocket
was CONNECTING (mid-reconnect), the send('leave') in disconnect() was
silently dropped — WebSocketClient.send() no-ops when readyState != OPEN.
The session would then sit on the server until the pong timeout (~87s).
Add sendLeaveAndClose() to WebSocketClient: OPEN → send + close immediately;
CONNECTING → mark closed (stops further reconnects), queue once('open', ...)
to send leave as soon as the socket opens; CLOSING/CLOSED → just cleanup.
Also fix the ReconnectTimeout error handler in client.ts, which was nulling
this.ws before calling disconnect(), causing disconnect() to skip the leave
entirely (the if (this.ws) guard would be false).
Guard startPingInterval() against this.closed to prevent a zombie interval
when close() is called synchronously from inside the once('open', ...) cb.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1253 +/- ##
==========================================
+ Coverage 28.00% 28.05% +0.05%
==========================================
Files 241 241
Lines 13854 13867 +13
Branches 1637 1641 +4
==========================================
+ Hits 3880 3891 +11
- Misses 9543 9544 +1
- Partials 431 432 +1
🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWebSocketClient now provides state-aware leave-and-close teardown, suppresses ping setup after closure, and is covered across WebSocket states. CallsClient uses this method during disconnect, ignores joins after closure, and retains its WebSocket reference for native errors. ChangesWebSocket teardown
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CallsClient
participant WebSocketClient
participant WebSocket
CallsClient->>WebSocketClient: sendLeaveAndClose()
WebSocketClient->>WebSocket: send leave and close when OPEN
WebSocketClient->>WebSocket: wait for open when CONNECTING
WebSocket-->>WebSocketClient: open event
WebSocketClient->>WebSocket: send leave and close
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webapp/src/websocket.test.ts (1)
546-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise the code path it claims to guard.
This test never invokes
mockWebSocket.onopen.startPingInterval()(the function thethis.closedguard was added to) is only ever called from insidews.onopeninwebsocket.ts— it's never reached via thehellomessage path used here. SincepingIntervalstarts asnulland is never set in this test, the final assertion passes trivially regardless of whether the guard exists.To actually validate the fix, the reconnect-
onopenpath needs to run (sostartPingInterval()executes) interleaved with the synchronousclose()triggered by the queuedopenlistener — e.g. by simulating a reconnect (isReconnect = true) and invokingmockWebSocket.onopen!(new Event('open'))rather than onlyonmessage. As written, this test would still pass even if theif (this.closed) { return; }guard instartPingInterval()were removed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webapp/src/websocket.test.ts` around lines 546 - 559, Update the test to exercise the reconnect onopen path that calls startPingInterval: configure the client/mock as a reconnect, invoke mockWebSocket.onopen!(new Event('open')) after triggering sendLeaveAndClose so the queued close runs synchronously, and then assert pingInterval remains null. Remove the hello onmessage simulation, ensuring the assertion fails if the closed guard in startPingInterval is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/websocket.ts`:
- Around line 188-213: Guard the join handling path during teardown: update the
ws.on('join') handler to immediately ignore or close when this.closed indicates
disconnection, preventing late acknowledgements from rebuilding RTCPeer. Ensure
sendLeaveAndClose's queued leave remains effective even if an existing open
listener runs first, and do not rely solely on removing open listeners.
---
Nitpick comments:
In `@webapp/src/websocket.test.ts`:
- Around line 546-559: Update the test to exercise the reconnect onopen path
that calls startPingInterval: configure the client/mock as a reconnect, invoke
mockWebSocket.onopen!(new Event('open')) after triggering sendLeaveAndClose so
the queued close runs synchronously, and then assert pingInterval remains null.
Remove the hello onmessage simulation, ensuring the assertion fails if the
closed guard in startPingInterval is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad02ca30-d4de-4180-a6f0-f3567abfa8b3
📒 Files selected for processing (3)
webapp/src/client.tswebapp/src/websocket.test.tswebapp/src/websocket.ts
3 participants
Summary
sendLeaveAndClose()toWebSocketClientthat correctly handles all three readyState cases: OPEN (send leave + close immediately), CONNECTING/mid-reconnect (mark closed to stop further reconnects, queueonce('open', …)to send leave as soon as the socket opens), CLOSING/CLOSED (just cleanup).this.ws = nullin theReconnectTimeouterror handler inclient.tsthat causeddisconnect()to silently skip the leave message.startPingInterval()againstthis.closedto prevent a zombie interval whenclose()is called synchronously from inside theonce('open', …)callback.Worst case without this fix: leave is never delivered and the server reaps the
CallSessionrow via pong timeout (~87s). With the fix, teardown is near-immediate.This is the v1/RTCD backport of the fix made in the v2/LiveKit codebase via PR #1250 (MM-69672).
Test plan
WebSocketClienttests passsendLeaveAndCloseunit tests covering OPEN, CONNECTING (queued leave delivered on open), CONNECTING (no zombie ping interval), CLOSED, CLOSING