Skip to content

fix: prevent parallel reconnect loops by storing and clearing timer references (OPTI-2438) #516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devin-ai-integration wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: prevent parallel reconnect loops by storing and clearing timer references (OPTI-2438) #516

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bd1fe0
fix: prevent parallel reconnect loops by storing and clearing timer r…
devin-ai-integration[bot] Apr 24, 2026
98d4fb4
fix: only increment reconnect generation for non-no-op calls
devin-ai-integration[bot] Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/millicast-sdk/src/utils/BaseWebRTC.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ export default class BaseWebRTC extends EventEmitter {
this.isReconnecting = false
this.tokenGenerator = tokenGenerator
this.options = null
this._disconnectTimerId = null
this._retryTimerId = null
this._reconnectGeneration = 0
}

/**
Expand All @@ -66,6 +69,10 @@ export default class BaseWebRTC extends EventEmitter {
*/
stop () {
logger.info('Stopping')
clearTimeout(this._disconnectTimerId)
clearTimeout(this._retryTimerId)
this._disconnectTimerId = null
this._retryTimerId = null
this.webRTCPeer.closeRTCPeer()
this.signaling?.close()
this.signaling = null
Expand Down Expand Up @@ -106,7 +113,7 @@ export default class BaseWebRTC extends EventEmitter {
this.reconnect({ error: new Error('Connection state change: RTCPeerConnectionState disconnected') })
} else if (state === 'disconnected') {
this.alreadyDisconnected = true
setTimeout(() => this.reconnect({ error: new Error('Connection state change: RTCPeerConnectionState disconnected') }), 1500)

@Yousif-CS Yousif-CS Apr 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we actually need a separate timer when we receive a disconnected, why couldn't we immediately invoke the reconnect here? Seems like 1500ms is a hack of some sorts. Either way it should work the way it is.

@devin-ai-integration devin-ai-integration Bot Apr 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good question. The 1500ms delay is pre-existing behavior that this PR preserves intentionally. The original intent appears to be giving the WebRTC ICE agent time to recover from a transient 'disconnected' state before triggering a full reconnect — ICE can sometimes transition disconnected → connected on its own if it's just a brief network blip. The immediate path (line 113) only fires when alreadyDisconnected && firstReconnection, i.e., the second consecutive disconnect or a 'failed' state, which are more definitive signals.

Agreed it feels like a heuristic/hack — a cleaner approach might be to wait for 'failed' exclusively rather than using a time-based guess. But that's a behavioral change worth its own discussion/ticket. This PR keeps the scope narrow to just fixing the timer leak and race condition.

@craig-johnston craig-johnston Apr 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@Yousif-CS if you want more changes (e.g. changing this to align), just ask for them. Devin will do them for you.

this._disconnectTimerId = setTimeout(() => this.reconnect({ error: new Error('Connection state change: RTCPeerConnectionState disconnected') }), 1500)
} else {
this.alreadyDisconnected = false
}
Expand All @@ -121,6 +128,11 @@ export default class BaseWebRTC extends EventEmitter {
* @property {String} error - The value sent in the first [reconnect event]{@link BaseWebRTC#event:reconnect} within the error key of the payload
*/
async reconnect (data) {
clearTimeout(this._disconnectTimerId)
clearTimeout(this._retryTimerId)
this._disconnectTimerId = null
this._retryTimerId = null
const generation = ++this._reconnectGeneration
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Outdated
try {
logger.info('Attempting to reconnect...')
if (!this.isActive() && !this.stopReconnection && !this.isReconnecting) {
Expand All @@ -146,7 +158,11 @@ export default class BaseWebRTC extends EventEmitter {
this.isReconnecting = false
this.reconnectionInterval = nextReconnectInterval(this.reconnectionInterval)
logger.error(`Reconnection failed, retrying in ${this.reconnectionInterval}ms. `, error)
setTimeout(() => this.reconnect({ error }), this.reconnectionInterval)
this._retryTimerId = setTimeout(() => {
if (this._reconnectGeneration === generation) {
this.reconnect({ error })
}
}, this.reconnectionInterval)
}
}
}
Expand Down
Loading