Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 17 additions & 3 deletions packages/@webex/internal-plugin-mercury/src/mercury.js
Original file line number Diff line number Diff line change
Expand Up @@ -901,9 +901,23 @@ const Mercury = WebexPlugin.extend({
break;
case 4000:
// metric: disconnect
this.logger.info(`${this.namespace}: socket ${sessionId} replaced; will not reconnect`);
if (isActiveSocket) this._emit(sessionId, 'offline.replaced', event);
// If not active, nothing to do
if (isActiveSocket) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this doesn't look right - we're changing the behavior for the isActiveSocket case. isActiveSocket is primarily for handling LLM where we can have multiple LLM connections, for Mercury we always have just one, so you would never have isActiveSocket==false for Mercury.
Also the testing done in the vidcast is not representative of the real world scenario, because it's setting up both mercury connections with same WDM in the same SDK instance. In real life it's 2 web apps running in separate windows, so 2 separate SDK instances each one has just a single Mercury connection.

// 4000 received directly from Mercury on the currently active socket.
// Treat as a transient disconnect and reconnect to recover.
this.logger.info(
`${this.namespace}: active socket ${sessionId} closed with 4000; reconnecting`
);
this._emit(sessionId, 'offline.transient', event);
Comment thread
mkesavan13 marked this conversation as resolved.
this._reconnect(socketUrl, sessionId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid reconnecting on replacement closes

When a 4000 close is caused by another tab/process replacing this Mercury connection, this socket is still the active socket inside the current SDK instance because the replacement is not in this.sockets; socket-base.js also normalizes close reason replaced to code 4000. Reconnecting here makes the replaced client immediately create a new connection and can kick the newer client off (or loop) instead of surfacing offline.replaced. Please only reconnect when the 4000 can be distinguished as a rebalance/transient close, not for generic replacements.

Useful? React with 👍 / 👎.

} else {
// 4000 received on an old/replaced connection (e.g. the SDK connected
// twice). The old socket is closed; the active connection remains and
// no reconnect should be triggered.
this.logger.info(
`${this.namespace}: old socket ${sessionId} replaced; will not reconnect`
);
this._emit(sessionId, 'offline.replaced', event);
Comment on lines +916 to +919

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve active socket on old 4000 closes

When an old socket closes with 4000 after a newer socket has been stored for the same session, _onclose has already run this.sockets.delete(sessionId) before reaching this branch, so the map entry that gets removed is the newer active socket. This path then emits offline.replaced and skips reconnect while leaving this.sockets empty; later getSocket(), disconnectAll(), and the active socket's own close cannot find it, so the SDK can leak the live socket or fail to recover when it closes. Only delete the map entry for the active socket, or re-store the active socket before taking this branch.

Useful? React with 👍 / 👎.

}
break;
case 4001:
// replaced during shutdown
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ describe('plugin-mercury', () => {
},
{
code: 4000,
action: 'replace',
action: 'reconnect',
},
{
action: 'close',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1394,6 +1394,75 @@ describe('plugin-mercury', () => {
});
});

describe('#_onclose() with code 4000 (replaced connection)', () => {
let mockSocket, anotherSocket;

beforeEach(() => {
mockSocket = {
url: 'ws://active-socket.com',
removeAllListeners: sinon.stub(),
};
anotherSocket = {
url: 'ws://old-socket.com',
removeAllListeners: sinon.stub(),
};
mercury.socket = mockSocket;
mercury.sockets.set(mercury.defaultSessionId, mockSocket);
mercury.connected = true;
sinon.stub(mercury, '_emit');
sinon.stub(mercury, '_reconnect');
sinon.stub(mercury, 'unset');
});

afterEach(() => {
mercury._emit.restore();
mercury._reconnect.restore();
mercury.unset.restore();
});

it('should reconnect when the active socket is closed with 4000', () => {
const closeEvent = {
code: 4000,
reason: 'replaced',
};

mercury._onclose(mercury.defaultSessionId, closeEvent, mockSocket);

assert.calledWith(mercury._emit, mercury.defaultSessionId, 'offline.transient', closeEvent);
assert.calledOnceWithExactly(
mercury._reconnect,
mockSocket.url,
mercury.defaultSessionId
);
assert.isFalse(mercury.connected);
});

it('should not reconnect when a non-active (old) socket is closed with 4000', () => {
const closeEvent = {
code: 4000,
reason: 'replaced',
};

mercury._onclose(mercury.defaultSessionId, closeEvent, anotherSocket);

assert.calledWith(mercury._emit, mercury.defaultSessionId, 'offline.replaced', closeEvent);
assert.notCalled(mercury._reconnect);
assert.isTrue(mercury.connected); // active connection remains
assert.notCalled(mercury.unset);
});

it('should clean up listeners from the old socket closed with 4000', () => {
const closeEvent = {
code: 4000,
reason: 'replaced',
};

mercury._onclose(mercury.defaultSessionId, closeEvent, anotherSocket);

assert.calledOnce(anotherSocket.removeAllListeners);
});
});

describe('shutdown switchover with retry logic', () => {
let connectWithBackoffStub;
const sessionId = 'mercury-default-session';
Expand Down