fix: reachability resolves only after all results instead of 1 per cl… #4438
Conversation
…uster and protocol
📝 WalkthroughWalkthroughThe reachability logic updates expected results counting to add 1 per cluster per protocol when at least one URL exists (otherwise 0), instead of summing all URLs. This applies to UDP for all clusters and TCP/XTLS for public clusters. Result accumulation remains unchanged. Tests were updated and expanded: import for ReachabilityResultsForBackend now comes from reachability.types, and new scenarios cover multiple URLs per protocol, clusters with zero URLs for certain protocols, configuration flags, timing, and metrics verification. No exported APIs changed beyond the type’s module relocation. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
marcin-bazyl
added
the
validated
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![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
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (2)
838-937: Great coverage for multi-URL per protocol; validates first-result readinessThis scenario effectively proves the fix: readiness occurs after the first result per protocol across clusters, ignoring surplus URLs. The expected min/max/avg metrics also correctly derive from the first results.
Consider adding one more assertion that reachability:done is emitted exactly once (you already count it elsewhere), to make this case self-contained.
940-991: Solid edge-case: zero TCP URLs does not block readinessGood validation that TCP is marked untested when no URLs exist and that readiness ignores that protocol. Metrics expectations for TCP being -1 across the board are correct.
As a complementary case, you might also add a VMN cluster with zero UDP URLs to ensure VMN readiness isn’t expected for that cluster (expectedResultsCount.videoMesh.udp += 0).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/reachability/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(2 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (2)
13-13: Import path update looks goodUsing ReachabilityResultsForBackend from reachability.types reflects the type relocation and keeps the public API unchanged.
6-6: Import path stabilityImporting Reachability from the directory path (index) is fine and consistent with the module structure.
|
Can we please update the PR title, the commit message is too long and it is getting truncated in the PR title. Would be helpfull to have a complete title for the changelog |
COMPLETES #SPARK-702198
This pull request addresses
Reachability is waiting for results from all urls instead of just the first one for each protocol and cluster, while ClusterReachability class emits the
resultReadyevent only once per protocol, so we never reach expected result count and instead resolve on the 3s timeout.by making the following changes
changed the number of expected results
Change Type
The following scenarios where tested
unit tests
The GAI Coding Policy And Copyright Annotation Best Practices
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.