fix(internal-plugin-mercury): improve handling of 4000 close code - #5065
fix(internal-plugin-mercury): improve handling of 4000 close code#5065rarajes2 wants to merge 2 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2764a46c07
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.logger.info( | ||
| `${this.namespace}: old socket ${sessionId} replaced; will not reconnect` | ||
| ); | ||
| this._emit(sessionId, 'offline.replaced', event); |
There was a problem hiding this comment.
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 👍 / 👎.
mkesavan13
left a comment
mkesavan13
left a comment
There was a problem hiding this comment.
Approved for the code changes. Needs to be merged only after doing a cross testing with Mercury team in INT
mkesavan13
added
validated
github-actions
Bot
removed
the
validated
Shreyas281299
added
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2cee02d2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `${this.namespace}: active socket ${sessionId} closed with 4000; reconnecting` | ||
| ); | ||
| this._emit(sessionId, 'offline.transient', event); | ||
| this._reconnect(socketUrl, sessionId); |
There was a problem hiding this comment.
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 👍 / 👎.
| 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) { |
There was a problem hiding this comment.
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.
5 participants
COMPLETES #< SPARK-808019 >
This pull request addresses
Fixes the mercury close code 4000 handling for server rebalancing.
by making the following changes
this.sockets, the map holds the new socket while the old socket still fires its close. SoisActiveSocketisfalse→ the old socket's listeners are cleaned up (by the existing top-level logic),offline.replacedis emitted, and no reconnect is triggered. The active connection stays alive.sourceSocket === sessionSocket→isActiveSocketistrue→ emitsoffline.transientand calls_reconnect(socketUrl, sessionId)to recover the connection.Change Type
The following scenarios were tested
mercury-events.js: changed the parametrized 4000 expectation fromreplacetoreconnect(that test closes the active socket).mercury.js: added a new#_onclose() with code 4000 (replaced connection)describe block with three cases — active socket reconnects, non-active (old) socket does not reconnect and stays connected, and old socket listeners are cleaned up.Note: I ran
build:srcsince the unit tests execute againstdist/.Vidcast: 4000 close code coming from the server (rebalancing case)
https://app.vidcast.io/share/f487df24-2e47-4cfd-9bb3-42d07e7fd7c3
Vidcast: Mercury 4000 close code testing for two times connect
https://app.vidcast.io/share/68fa87d8-2d4c-4125-b6e8-8cb6b18b8aa3
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.