fix: set stopReconnection before closeRTCPeer in stop() to prevent race condition (OPTI-2440)

[devin-ai-integration]

Summary

Reorders BaseWebRTC.stop() so that this.stopReconnection = true is set before calling this.webRTCPeer.closeRTCPeer(), rather than after.

closeRTCPeer() can synchronously emit connectionStateChange events (PeerConnection.js line 83 explicitly emits 'closed', and the native peer.close() may fire onconnectionstatechange with 'disconnected' depending on browser implementation). The setReconnect() listener processes these events and can call reconnect(), which checks !this.stopReconnection as a guard. With the old ordering, stopReconnection was still false when these events fired, potentially allowing an unintended reconnect attempt after an explicit stop().

The trigger requires: autoReconnect=true, a prior 'disconnected' event having set alreadyDisconnected=true, and firstReconnection=true — a narrow but realistic scenario (e.g., user calls stop() shortly after a network flap).

Jira: OPTI-2440

Teardown Safety Verification

All code paths triggered during stop() were traced to confirm nothing reads stopReconnection during teardown and expects it to be false:

Teardown step	What it does	Reads stopReconnection?
closeRTCPeer() → peer?.close()	Native browser call; may fire onconnectionstatechange synchronously on the PeerConnection EventEmitter	No
closeRTCPeer() → this.peer = null	Nulls the native peer	No
closeRTCPeer() → this.stopStats()	Stops stats collection, nulls peerConnectionStats	No
closeRTCPeer() → emit('closed')	Listener at BaseWebRTC.js:99 receives 'closed' — does NOT match 'failed' or 'disconnected' conditions (line 104/107), falls to else branch which just sets alreadyDisconnected = false	No
signaling.close()	Calls WebSocket.close(), which fires connectionClose — the reconnect listener only listens for connectionError, not connectionClose	No
View.stop() / Publish.stop() overrides	Call super.stop() first, then clear DRM maps / terminate workers — none reference stopReconnection	No

All 6 references to stopReconnection across the codebase were checked:

• BaseWebRTC.js:50 — constructor init (false) — not during teardown
• BaseWebRTC.js:69 — stop() sets true — this is our fix
• BaseWebRTC.js:126 — reconnect() guard: !this.stopReconnection — the only consumer; true earlier is strictly safer
• Publish.js:164 / View.js:244 — initConnection() resets to false — runs during connect(), not teardown
• Publish.js:185 / View.js:270 — sets true on 401 error — not during teardown

Conclusion: The reorder is safe. No downstream teardown code depends on stopReconnection being false.

Review & Testing Checklist for Human

• Verify no teardown logic depends on stopReconnection being false — Traced above. All teardown paths through closeRTCPeer(), signaling.close(), and subclass overrides confirmed safe.
• Validate the race condition scenario is real — The synchronous event emission path through PeerConnection.closeRTCPeer() → addPeerEvents listeners → BaseWebRTC.setReconnect() → BaseWebRTC.reconnect() is confirmed. Whether browsers fire 'disconnected' synchronously during peer.close() is implementation-dependent, but the SDK's own emit('closed') on line 83 is unconditionally synchronous.
• Consider adding a regression test: The existing mock RTCPeerConnection.close() does not fire onconnectionstatechange, so this race is not covered by the test suite. A test that makes the mock emit 'disconnected' synchronously during close() would guard against future regressions.
• Spot-check the trace above: The safety argument rests on the teardown analysis. A reviewer should independently verify that no listener or callback path was missed — particularly any connectionStateChange handlers registered outside of setReconnect() (e.g., the migration handler in Publish.js:271-277, which only fires on 'connected'/'disconnected'/'failed'/'closed' and just closes old instances — safe).

Notes

• This is a one-line reorder with no new code, but the correctness argument depends on understanding the synchronous event emission path through PeerConnection.closeRTCPeer() → addPeerEvents listeners → BaseWebRTC.setReconnect() → BaseWebRTC.reconnect().
• The async keyword on closeRTCPeer() is misleading — the method body is entirely synchronous. This doesn't affect the fix but is worth noting.
• The recursive stop() call inside reconnect() (line 127) is harmless with the new ordering — setting stopReconnection = true again is idempotent.

Link to Devin session: https://dolby.devinenterprise.com/sessions/56aeae174c444e3ab952f7b1b8257c91
Requested by: @craig-johnston

---

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7846f37

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.