fix(mobius-socket): add async event dedup#4880
fix(mobius-socket): add async event dedup#4880rsarika wants to merge 3 commits intowebex:mobius-socketfrom
Conversation
rsarika
added
validated
![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: 6b12c2b9e5
ℹ️ 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".
| const sessionSocket = this.sockets.get(sessionId); | ||
| const suffix = sessionId === this.defaultSessionId ? '' : `:${sessionId}`; | ||
|
|
||
| this._clearSeenAsyncEventIds(sessionId); |
There was a problem hiding this comment.
_seenAsyncEventIdsBySession is only purged from explicit disconnect()/disconnectAll() paths, so sessions that are closed by the server and never explicitly disconnected keep their dedup Map indefinitely. In _onclose() we already remove this.sockets entries for those sessions, but the new dedup cache remains reachable and can grow to dedupCacheMaxSize per stale session ID, which creates an unbounded memory leak in multi-session deployments over time.
Useful? React with 👍 / 👎.
…bius-socket-dedup
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ab823af82
ℹ️ 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".
| * duplicate async_event messages retransmitted by the server. | ||
| * @type {Number} | ||
| */ | ||
| dedupCacheMaxSize: process.env.MOBIUS_SOCKET_DEDUP_CACHE_MAX_SIZE || 1000, |
There was a problem hiding this comment.
dedupCacheMaxSize is read directly from process.env as a raw string, but _shouldSuppressDuplicateAsyncEvent() compares Map.size numerically against it. If this env var is set to a non-numeric value (for example from a typo), the comparison becomes size > NaN, eviction never runs, and the per-session dedup map can grow unbounded. Please coerce/validate this value to a finite number with a safe fallback so the cache bound is always enforced.
Useful? React with 👍 / 👎.
mkesavan13
left a comment
mkesavan13
left a comment
There was a problem hiding this comment.
Just a couple of nitpick comments. Approving.
Could you please add a VidCast as well before merging the PR?
| if (!seenAsyncEventIds) { | ||
| seenAsyncEventIds = new Map(); | ||
| this._seenAsyncEventIdsBySession.set(sessionId, seenAsyncEventIds); | ||
| } |
There was a problem hiding this comment.
Shouldn't this be treated as just get and set should happen only in set?
| this._seenAsyncEventIdsBySession.clear(); | ||
| }, | ||
|
|
||
| _shouldSuppressDuplicateAsyncEvent(sessionId, envelope) { |
There was a problem hiding this comment.
May be a rename to reflect the action would help as it does not just check but also set if it is not preexisting.
2 participants
COMPLETES #https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7843
This pull request addresses
Duplicate
async_eventmessages can be retransmitted by Mobius, which can cause the same event to be processed multiple times by consumers during an active session.by making the following changes
async_eventmessages inMobiusSocket, keyed byeventIdMobiusSocketlayer so duplicate suppression survives socket replacement for the same sessiondisconnect()and all caches ondisconnectAll()async_eventmessagesChange Type
The following scenarios were tested
typeandeventIdgit diff --checkeslinton the updated source filesNote: the package test run completed successfully for the targeted Mocha tests, but the sandbox also prints unrelated
EPERMnoise from the helper server attempting to bind to0.0.0.0:8000.The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.