fix: locus DTOs not handled if an earlier one causes an error #3982
Conversation
WalkthroughThe pull request introduces several modifications across the Changes
Possibly related PRs
Suggested reviewers
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) 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/@webex/plugin-meetings/src/locus-info/parser.ts (3)
774-781: Enhance error handling with metrics and better error reporting.The addition of error handling is good and aligns with the PR objectives. However, consider these improvements:
Apply this diff to enhance the error handling:
try { this.onDeltaAction(lociComparison, newLoci); } catch (error) { + // Send metric to track error frequency + Metrics.sendBehavioralMetric(BEHAVIORAL_METRICS.LOCUS_DELTA_ACTION_ERROR, { + error: error.message, + comparison: lociComparison, + stack: error.stack + }); + LoggerProxy.logger.error( - 'Locus-info:parser#processDeltaEvent --> Error in onDeltaAction', + 'Locus-info:parser#processDeltaEvent --> Error processing delta action', + { + error, + comparison: lociComparison, + delta: Parser.locus2string(newLoci) + } ); + + // Emit error event for higher-level error handling + this.emit('error', { + type: 'DELTA_ACTION_ERROR', + error, + comparison: lociComparison, + delta: newLoci + }); }These changes will:
- Track error frequency using metrics
- Provide more context in error logs
- Allow higher-level components to handle errors through events
Line range hint
632-782: Consider refactoringprocessDeltaEventfor better maintainability.The method is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused methods.
Here are some suggestions for improvement:
- Extract the locus comparison logic into a separate method:
private handleLocusComparison(lociComparison: string, newLoci: LocusDeltaDto): boolean { const {DESYNC, USE_INCOMING, WAIT, LOCUS_URL_CHANGED} = Parser.loci; switch (lociComparison) { case DESYNC: this.pause(); return false; case USE_INCOMING: case LOCUS_URL_CHANGED: this.workingCopy = newLoci; return false; case WAIT: this.queue.enqueue(newLoci); return true; default: return false; } }
- Extract the wait state handling into a separate method:
private handleWaitState(needToWait: boolean, newLoci: LocusDeltaDto): void { if (needToWait) { this.status = 'BLOCKED'; this.startSyncTimer(); } else { this.stopSyncTimer(); if (this.status === 'BLOCKED') { this.status = 'WORKING'; LoggerProxy.logger.info( `Locus-info:parser#processDeltaEvent --> received delta that we were waiting for ${Parser.locus2string( newLoci )}, not blocked anymore` ); } } }
- Add stronger typing for the parser status:
type ParserStatus = 'IDLE' | 'PAUSED' | 'WORKING' | 'BLOCKED';These refactoring suggestions would make the code more maintainable and easier to test.
Line range hint
509-524: Improve error handling for invalid locus objects.The current error handling for invalid locus objects could be enhanced with better error reporting and metrics.
Apply this diff to improve the error handling:
isValidLocus(newLoci) { let isValid = false; const {isLoci} = Parser; // one or both objects are not locus delta events if (!isLoci(this.workingCopy) || !isLoci(newLoci)) { + const reason = !isLoci(this.workingCopy) ? 'invalid working copy' : 'invalid new locus'; + Metrics.sendBehavioralMetric(BEHAVIORAL_METRICS.INVALID_LOCUS_OBJECT, { + reason, + stack: new Error().stack + }); LoggerProxy.logger.info( - 'Locus-info:parser#processDeltaEvent --> Ignoring non-locus object. workingCopy:', - this.workingCopy, - 'newLoci:', - newLoci + 'Locus-info:parser#processDeltaEvent --> Ignoring non-locus object', + { + reason, + workingCopy: this.workingCopy, + newLoci + } ); } else { isValid = true; } return isValid; }This change will:
- Track invalid locus objects with metrics
- Provide better context about which object is invalid
- Structure the log message better
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
1810-1837: Test case looks good but could be enhanced.The test case correctly verifies that the system can handle subsequent Locus delta events after an error, which aligns with the PR objectives. However, consider enhancing it with additional assertions and error scenarios.
Consider adding:
- Assertions to verify error handling:
locusInfo.handleLocusDelta(fakeLocusDelta, testMeeting); +assert.throws(() => locusParser.onDeltaAction(), Error, 'fake error'); // simulate a second locus delta coming - it should be processed without errors locusInfo.handleLocusDelta(fakeLocusDelta, testMeeting); +assert.calledTwice(locusParser.onDeltaAction);
- Additional test cases for different error scenarios:
it('handles different types of errors during delta processing', () => { // Test TypeError locusParser.onDeltaAction = sandbox.stub() .onCall(0) .throws(new TypeError('type error')); // Test RangeError locusParser.onDeltaAction = sandbox.stub() .onCall(0) .throws(new RangeError('range error')); }); it('handles async errors during delta processing', async () => { locusParser.onDeltaAction = sandbox.stub() .onCall(0) .rejects(new Error('async error')); });packages/@webex/internal-plugin-mercury/src/mercury.js (2)
Line range hint
219-219: Storing the last connection error for diagnosticsSetting
this.lastErrorin the_attemptConnectionmethod allows retrieval of the most recent connection error viagetLastError(). This is helpful for debugging and monitoring connection issues. Ensure thatlastErroris cleared or updated appropriately to reflect the current state.
Line range hint
479-481: Consistent use of thereasonvariable in loggingIn the
_onclosemethod, consider using thereasonvariable instead ofevent.reasonin the log message for consistency, sincereasonhas been processed (e.g., transformed to lowercase). This ensures the logged reason matches the one used in the reconnection logic.Apply this diff to address the inconsistency:
this.logger.info( - `${this.namespace}: socket disconnected; will not reconnect: ${event.reason}` + `${this.namespace}: socket disconnected; will not reconnect: ${reason}` );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/@webex/internal-plugin-mercury/src/mercury.js(1 hunks)packages/@webex/plugin-meetings/src/locus-info/parser.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js(1 hunks)
🔇 Additional comments (3)
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
Line range hint 1-100: Well-structured test file with good organization.
The test file follows testing best practices with:
- Clear test organization using describe blocks
- Consistent setup/teardown patterns
- Descriptive test names
- Good use of stubs and spies
packages/@webex/internal-plugin-mercury/src/mercury.js (2)
Line range hint 45-57: Efficient management of concurrent connections with @oneFlight decorator
The use of the @oneFlight decorator on the connect method ensures that simultaneous calls to connect are handled gracefully, preventing multiple concurrent connection attempts. This improves the stability of the connection logic.
Line range hint 67-77: Safe handling of multiple disconnection attempts
Applying the @oneFlight decorator to the disconnect method effectively manages concurrent calls to disconnect, avoiding potential race conditions or inconsistent states during disconnection.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
marcin-bazyl
added
the
validated
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js (1)
Line range hint
349-360: Avoid commenting out test cases forNotFounderrorsThe test case handling the
NotFounderror has been commented out. Commenting out tests can reduce test coverage and may lead to unhandled error scenarios. If this test is no longer applicable, consider removing it entirely. If the functionality is still required, update and re-enable the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js(1 hunks)
🔇 Additional comments (2)
packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js (2)
Line range hint 11-11: Verify the commented-out NotFound import
The NotFound import has been commented out. Please confirm whether handling for NotFound errors is intentionally being removed. If NotFound errors are still relevant, ensure they are properly handled and tested.
709-715: Test correctly asserts enhanced error logging in _emit method
The added test ensures that mercury.logger.error is called with the expected arguments when an error occurs in an event handler, which improves debugging and log clarity.
COMPLETES #SPARK-559101
This pull request addresses
When an error is thrown while processing some Locus DTO, SDK gets stuck and will not process any further updates from Locus.
by making the following changes
Catching the error in the right place, so that the LocusInfo parser state is updated and doesn't get stuck on "WORKING" preventing any further locus DTOs from being processed.
Also fixed an error log in mercury, because it was not really showing the error that was caught - intead it would show:
error={}Change Type
The following scenarios where tested
unit tests, manual test with web app
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
listen()andstopListening()in favor ofconnect()anddisconnect(), respectively.