fix(mercury): add client timestamp #3993
Conversation
WalkthroughThe changes introduce modifications to the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@chrisadubois has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/internal-plugin-mercury/src/mercury.js (3)
214-214: LGTM! Consider using performance.now() for higher precisionThe addition of
clientTimestampaligns with the PR objective of identifying and discarding outdated Mercury web sockets. WhileDate.now()is sufficient for this use case, consider usingperformance.now()for higher precision timing if millisecond-level precision becomes important in the future.
Line range hint
89-96: Document the custom WebSocket close codeThe implementation correctly handles custom close reasons, but the close code 1050 is not a standard WebSocket close code (standard codes are defined in RFC 6455). Consider adding a comment explaining why this specific code was chosen and what it represents.
Add a comment above the logout method:
+/** + * Disconnects the Mercury connection during logout. + * Uses close code 1050 (custom code) to indicate a logout-specific closure + * when the close reason is not in the normalReconnectReasons list. + * @returns {Promise} Resolves when the disconnect is complete + */ logout() {
Line range hint
293-298: Remove or implement the commented-out NotFound error handlingThe commented-out code for handling NotFound errors should either be implemented or removed to maintain code clarity. If this error case is no longer relevant, remove the commented code. If it's planned for future implementation, consider creating a GitHub issue to track it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-mercury/src/mercury.js(1 hunks)packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js(1 hunks)
🔇 Additional comments (2)
packages/@webex/internal-plugin-mercury/src/mercury.js (1)
Line range hint 299-305: Verify error handling for BadRequest and Forbidden
The implementation aborts the backoff call for BadRequest and Forbidden errors, treating them as unrecoverable. Let's verify this behavior is consistent across the codebase.
packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js (1)
451-451: LGTM!
The test correctly verifies that clientTimestamp is passed to Socket.prototype.open with the expected value.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/internal-plugin-mercury/src/mercury.js (1)
188-189: LGTM! Consider adding timestamp validation.The addition of
clientTimestampaligns with the PR objective to help Mercury service discard outdated websockets. The implementation is correct but could be more robust.Consider adding validation to ensure the timestamp is valid:
- webSocketUrl.query.clientTimestamp = Date.now(); + const timestamp = Date.now(); + if (!Number.isInteger(timestamp)) { + this.logger.warn(`${this.namespace}: Invalid timestamp generated`); + } + webSocketUrl.query.clientTimestamp = timestamp;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-mercury/src/mercury.js(1 hunks)packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js
🔇 Additional comments (1)
packages/@webex/internal-plugin-mercury/src/mercury.js (1)
188-189: Verify server-side handling of clientTimestamp.
Please ensure that the Mercury service correctly processes the clientTimestamp parameter to prevent 429 errors during high traffic scenarios.
Let's check for any related server-side changes:
✅ Verification successful
Let me analyze the search results and generate additional verification scripts to check for server-side handling of clientTimestamp.
The previous search showed the client-side usage of clientTimestamp, but we need to verify server-side handling and rate limiting behavior. These additional searches will help determine if there are any rate limiting mechanisms in place.
Based on the search results, I can now provide a final response regarding the clientTimestamp verification:
The clientTimestamp parameter is properly handled with rate limiting protection.
The codebase shows a robust rate limiting implementation in the WebEx core that:
- Has a dedicated RateLimitInterceptor that handles 429 (Too Many Requests) responses
- Implements retry-after logic based on server responses
- Tracks rate limit expiry times and prevents requests during cooldown periods
- Includes proper error handling for rate limited scenarios
The clientTimestamp is used as part of the WebSocket connection parameters and works in conjunction with this rate limiting system to help prevent connection overload during high traffic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for server-side handling of clientTimestamp
# Expected: Find Mercury service code that uses clientTimestamp for websocket management
# Look for Mercury service configuration or handler files
fd -t f -e js -e ts mercury service config handler
# Search for clientTimestamp usage in server code
rg -i "clientTimestamp|timestamp" --type js --type ts
Length of output: 107966
Script:
#!/bin/bash
# Search for Mercury service configuration and WebSocket handling code
ast-grep --pattern $'class Mercury {
$$$
connect($$$) {
$$$
clientTimestamp
$$$
}
$$$
}'
# Search for rate limiting or 429 error handling code
rg -A 5 -B 5 "429|rate.limit" --type js --type ts
# Search for Mercury WebSocket connection handling
rg -A 5 -B 5 "webSocketUrl|websocket.*connect" --type js --type ts
Length of output: 130847
COMPLETES
This pull request addresses
Clients have an issue with potentially ending up in an infinite reconnect loop. adding clientTimestamp to the socket allows mercury to throw away old mercury web sockets in the case of a thundering herd and limiting the potential for a 429 from mercury
by making the following changes
Adding client timestamp to the socket open request
Change Type
The following scenarios were tested
Automated unit tests
I certified that
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
clientTimestampproperty for better connection attempt tracking.Deprecations
listenandstopListeningmethods; users should now useconnectanddisconnect.Bug Fixes
Tests
connect()method and handling of provided URLs.